Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark new createPrinter vars as pure to remove new code from typingsInstaller #52438

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

jakebailey
Copy link
Member

#52382 added a few new top-level variables to emitter.ts; but, I noticed in dcad07f (which pulled this change into release-5.0) that typingsInstaller grew by 5000+ lines.

This is because the memoize function isn't seen as pure (it could reasonably call the function it accepts immediately). I don't want to totally throw off the bundle size, so just mark the new things as pure to restore the old size.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 26, 2023
@jakebailey
Copy link
Member Author

The alternative is to write these calls explicitly:

let somePrinter: Printer | undefined;
function createSomePrinter(): Printer {
	return somePrinter ??= createPrinter();
}

I did scan the repo and there was only one other top level memoize usage in services, on which @__PURE__ does nothing.

@DanielRosenwasser
Copy link
Member

Did you validate locally that this is smaller?

@jakebailey
Copy link
Member Author

I did, yep:

$ git diff --diff-filter=AM --no-index ./built/local-old/typingsInstaller.js ./built/local/typingsInstaller.js --numstat
1   5648    ./built/{local-old => local}/typingsInstaller.js

@sheetalkamat
Copy link
Member

Its interesting that this is not tree shaked out ? i thought you said things are getting removed if not used. I dont see why typingInstaller needs printer. But thats a separate question i guess..

@jakebailey
Copy link
Member Author

It's not tree shaken because the arrow function (which references createPrinter) is passed to another function (memoize), and it doesn't know that memorize doesn't immediately call the function or something. So it can't eliminate the memoize call because it could have a side effect.

@jakebailey
Copy link
Member Author

I would absolutely love a way to mark certain functions themselves as side-effect free, but that doesn't seem to be a thing that bundlers support (they all seem to do it at the function call site instead, which is a shame).

@jakebailey jakebailey merged commit f794fbf into microsoft:main Jan 27, 2023
@jakebailey jakebailey deleted the pure-create-emitter branch January 27, 2023 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants