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

Disable esparse's TS option to avoid removal of unused imports #87

Merged
merged 5 commits into from
Oct 14, 2022

Conversation

airportyh
Copy link
Contributor

@airportyh airportyh commented Oct 13, 2022

Why?

We have a bug where if you first run a file with an un-used import, say:

import isOdd from 'is-odd';

Then you update the code to use the import and run it again:

import isOdd from 'is-odd';
console.log(isOdd(3));

You get a module 'is-odd' is not found error because:

  1. upm failed to auto install the module during the 1st run because the unused import was striped by esparse (fork of esbuild) before the guesser could include the module
  2. in the second run, the guesser used the cached result from the first run because the hash key is based on import statements grabbed using regexp searches instead of eparse: https://github.com/replit/upm/blob/master/internal/store/util.go#L32

More context:
https://app.asana.com/0/1201647179266797/1202932490289607/f

What Changed?

  1. disabled the TS.Parse option of esparse
  2. disabled the MangleSyntax option of esparse
  3. the above 2 together disable the removal of unused imports in the parsing process: https://github.com/amasad/esparse/blob/master/parser/parser.go#L7614. Basically we put the parser into JS parse mode that can cope with TS but just ignores the semantics of TS. This is ok for what we want: just the list of import statements. This also has the nice effect that the user can write the statement: import something from 'something', run it, and get the library installed, which I think is more intuitive.

@airportyh airportyh changed the title Stop using regexps to calculate hash for nodejs Disable esparse's TS option to avoid removal of unused imports Oct 13, 2022
@airportyh airportyh added the boop label Oct 13, 2022
@replbot replbot added the bop label Oct 13, 2022
Copy link

@gburtini gburtini left a comment

Choose a reason for hiding this comment

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

@airportyh and I talked offline. He's going to add a few test cases here, one for a hard-to-parse TypeScript case and another for to catch this regression.

For posterity, this was originally introduced here and includes a comment about the speed (cost?) of React parsing. There are some existing tests on React-ish JS over here that we also considered.

@replbot replbot removed the boop label Oct 13, 2022
@airportyh
Copy link
Contributor Author

@airportyh and I talked offline. He's going to add a few test cases here, one for a hard-to-parse TypeScript case and another for to catch this regression.

Thanks! Turns out the original tests failed the moment I ran the original testcases as .ts files. Conditional import/requires don't work if they come after typings.

@airportyh airportyh merged commit 872ae29 into master Oct 14, 2022
@airportyh airportyh deleted the th-fix-nodejs-sticky-non-guess branch October 14, 2022 15:19
@blast-hardcheese blast-hardcheese added the bug Something isn't working label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bop bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants