FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Mark new createPrinter vars as pure to remove new code from typingsInstaller by jakebailey · Pull Request #52438 · microsoft/TypeScript · GitHub

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

Merged
Jake Bailey (jakebailey) merged 1 commit into
microsoft:mainfrom
jakebailey:pure-create-emitter
Jan 27, 2023
Merged

Mark new createPrinter vars as pure to remove new code from typingsInstaller#52438
Jake Bailey (jakebailey) merged 1 commit into
microsoft:mainfrom
jakebailey:pure-create-emitter

Conversation

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Member

Did you validate locally that this is smaller?

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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..

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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).

Jake Bailey (jakebailey) merged commit f794fbf into microsoft:main Jan 27, 2023
Jake Bailey (jakebailey) deleted the pure-create-emitter branch January 27, 2023 00:46
Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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


Back | FazBrowse Home | New Git URL