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

Remove regex replacements from import-typescript #3356

Merged
merged 4 commits into from
Oct 18, 2022

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Oct 11, 2022

Rather than modifying TS's source to try and remove require/module/debugger/etc, we can instead define those variables at the top and leave the source unmodified (which TS can handle), and then remove debugger at esbuild time.

This approach is a lot more flexible for TypeScript. We don't have to think about which random bits of code happen to be regex replaced here, and it could even mean that our package could be shipped minified or optimized in some way, if we end up doing that. Mainly, this means we don't have to add a whole slew of new special cases for the module-ified TS; this approach works for all modern versions of TS.

Supercedes #3352.

This includes #3344.

// ╵ ~~~~
//

tsServices = tsServices.replace(/\nvar ([^ ]+) = \(this && this\.([^)]+)\) \|\|/gm, '\nvar $1 =');
Copy link
Member Author

@jakebailey jakebailey Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does have an effect, but as far as I can tell esbuild doesn't complain when I run npm run release.

@jakebailey
Copy link
Member Author

This PR is now running on TypeScript-Make-Monaco-Builds and is working.

@alexdima
Copy link
Member

alexdima commented Oct 17, 2022

Very cool approach! The reason we removed require calls from the file was because webpack and other bundlers were picking them up and inlining nodejs shims in the resulting bundle. I will do a test-pass to see that all the bundlers we support (webpack, esbuild, vite, parcel) are still happy after this change.

@jakebailey
Copy link
Member Author

The reason we removed require calls from the file was because webpack and other bundlers were picking them up and inlining nodejs shims in the resulting bundle.

FWIW this shouldn't actually end up mattering for TypeScript internally; we detect that it's not a node system and never actually try and use those shims, if they weren't present. Of course, it's not so great that the bundle size would increase and/or warnings would appear.

@alexdima
Copy link
Member

All bundlers look happy! Thank you!

@alexdima alexdima merged commit 6e995f7 into microsoft:main Oct 18, 2022
@jakebailey jakebailey deleted the redo-require-removal branch October 18, 2022 19:28
@jakebailey
Copy link
Member Author

Thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants