Skip to content

Add external mapShim/debug modules #33712

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

Merged
merged 2 commits into from
Oct 7, 2019
Merged

Add external mapShim/debug modules #33712

merged 2 commits into from
Oct 7, 2019

Conversation

rbuckton
Copy link
Contributor

@rbuckton rbuckton commented Oct 1, 2019

This moves sys.require from tsserver to compiler so that NodeJS modules can be loaded via sys if available. This enables us to move the Map shim out of tsc.js, since it will be most likely run in a NodeJS environment that already supports it and will cut down on unused code in the command-line compiler. The Map shim script is designed such that it can be bundled with typescriptServices.js/typescript.js, allowing it to continue to run in environments that do not support module loading and may not have a native Map implementation.

This also enables us to dynamically load extended debugging tools when debugging the compiler without also adding code to the compiler that would be unused in most situations.

As part of this change I also moved all of the general path-manipulation logic out of utilities.ts and into a new path.ts file. This was motivated by the need to leverage some of the path manipulation logic before we evaluate scanner.ts/parser.ts which unconditionally create Map instances (and therefore would depend on sys.require and path manipulation already being present).

@rbuckton
Copy link
Contributor Author

rbuckton commented Oct 1, 2019

cc: @ahejlsberg as this adds the additional control-flow debug info via an on-demand-loaded external module.

@weswigham
Copy link
Member

weswigham commented Oct 1, 2019

The Map shim is essentially just for old ie runtimes at this point. Polyfilling it lazily using require defeats the point, since that runtime doesn't have require?

@rbuckton
Copy link
Contributor Author

rbuckton commented Oct 1, 2019

True, but IE will generally be running typescriptServices.js/typescript.js, which bundles mapShim. This is also the implementation I'm using to shim Promise in the plugin prototype branch.

@rbuckton
Copy link
Contributor Author

rbuckton commented Oct 1, 2019

Since tsc.js will never require() the map shim, and since typescriptServices will always bundle it, I'll remove the logic to try to require() it if the ts.createMapShim function can't be found and have it throw instead.

@rbuckton rbuckton force-pushed the externalShims branch 4 times, most recently from 006946d to a460720 Compare October 1, 2019 23:20
@rbuckton
Copy link
Contributor Author

rbuckton commented Oct 2, 2019

I lament that ts.Map<T> was defined with a single type argument and was made public. It should have been ts.Map<K, V> (or ts.Map<K extends string, V>) so that we could possibly move to using a full Map shim that supported number/boolean/object/etc. at a later date without having to use something like ts.ESMap<K, V>.

Speaking of which, I have a branch where I've improved the Map shim to allow for arbitrary keys (and has ideally the same storage size for string-only keyed maps), as well as added a Set shim. Its fairly efficient for keys that are string/number/boolean/null/undefined, but uses a linked-list for object keys. They obviously wouldn't be efficient for objects, but would in effect just be a safety net for running typescriptServices.js in IE.

@rbuckton
Copy link
Contributor Author

rbuckton commented Oct 2, 2019

@weswigham any other thoughts/comments?
@RyanCavanaugh any concerns with changing tsc.js to no longer ship with a polyfill for Map?

@rbuckton
Copy link
Contributor Author

rbuckton commented Oct 4, 2019

@weswigham: Any other feedback or concerns on this?

@rbuckton rbuckton merged commit 01b3d41 into master Oct 7, 2019
@rbuckton rbuckton deleted the externalShims branch October 7, 2019 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants