Replace babel parse with es-module-lexer #208

alex-saunders · 2020-02-14T23:14:38Z

This PR introduces es-module-lexer, and uses it to scan import statements when snowpack is ran with the --include flag enabled (replacing the functionality previously provided by @babel/parse and @babel/traverse).

I haven't added any new tests as the current suite of include-* tests seemed to cover all test cases that I could think of, however there were a couple of requirements I wasn't totally sure about (see my comments) so it might be worth introducing a couple of tests once the requirements are nailed down.

The speed increase from using a wasm-based lexer is definitely noticable! However, the information provided by es-module-lexer is a lot more minimal than what we were previously leveraging with babel so I've had to write some fairly rudimentary parsing checks to return what I believe to be the correct information per InstallTarget object returned by getInstallTargetsForFile

es-module-lexer also doesn't ship with any types, nor does it have a @types package, so I've included a basic module declaration file under the @types directory.

closes #146

src/scan-imports.ts

alex-saunders · 2020-02-14T23:34:50Z

Looks es-module-lexer's exports aren't compatbile with node 13.x. I've tested it locally and npm run build runs fine with node 12 and below but we're getting an Error: No valid exports main found error when trying to build on node 13 which is causing CI to fail. @FredKSchott any ideas? I fear we'll either have to get the es-module-lexer package updated or vendor it ourselves for the time being 😬

drwpow · 2020-02-14T23:50:36Z

Looks es-module-lexer's exports aren't compatbile with node 13.x

Does it work if you import es-module-lexer/dist/lexer.js instead of es-module-lexer?

drwpow · 2020-02-14T23:53:13Z

package.json

    "@babel/preset-env": "^7.6.3",
-    "@babel/traverse": "^7.6.3",


It’s a shame that we don’t gain anything from dropping these since they’re deps of other Babel modules! Still, it brings us closer to dropping Babel altogether which is cool

Yea for sure, the benefit here is definitely more speed & stability (a lot less can go wrong when you're just scanning and not also parsing it into a valid AST). But you're right that the total dep count probably hasn't dropped by that much. 🤷‍♂

FredKSchott · 2020-02-15T01:10:16Z

Looks es-module-lexer's exports aren't compatbile with node 13.x.

lol wow, it looks the issue is that the "require" export is 'dist/lexer.cjs', and not './dist/lexer.cjs'. That's a tricky one: https://nodejs.org/api/esm.html#esm_package_exports

https://github.com/guybedford/es-module-lexer/blob/master/package.json#L7-L10
It looks like that change was just merged to master in that repo, would you be able to create a PR to that repo explaining that their package is broken in Node v13? /cc @guybedford just for a heads up

alex-saunders · 2020-02-15T17:33:18Z

Ahh good spot @FredKSchott , thanks for looking into it! I'll open a PR with es-module-lexer now

drwpow · 2020-02-15T19:46:36Z

@types/es-module-lexer/index.d.ts

+  };
+  export function init(): Promise<void>;
+  export function parse(code: string): [Array<ImportSpecifier>, Array<string>];
+}


Nice! Maybe if you have time you could contribute these to https://github.com/DefinitelyTyped/DefinitelyTyped, then we can get help from others maintaining this (unless the package author would like to ship these types themselves)

Totally, that was my plan after I'd got the types nailed for our use case, will definitely be contributing them to DefinitelyTyped

drwpow · 2020-02-15T19:49:20Z

This is awesome! Just tested this out locally in a project and it worked great. --include seemed to work the exact same, just much faster 🙂

alex-saunders · 2020-02-15T20:17:51Z

es-module-lexer has been updated and the package now works with node 13. However, we now get a warning in the console every time snowpack is ran due to the experimental feature that es-module-lexer is using with conditional exports:

As far as I can tell, the only way to supress the warning is to add --no-warnings to the shebang of the CLI file but as this is generated automatically, I'm not quite sure how to tackle this!

Anyone got any ideas?

FredKSchott

Nice! Yea, this is a really tricky one. es-module-lexer is so powerful but that definitely comes at the expense of a fair bit of RegEx / substring magic.

I think the existing include test(s) should cover it, but maybe add/update to them to just make sure that they're covering the cases that we're worried about handling correctly:

import a, {b, c as d} from ...
import {b, c as d}, a from ...
import * as ns from
import a, * as ns from

src/index.ts

src/scan-imports.ts

FredKSchott · 2020-02-16T18:16:36Z

src/scan-imports.ts

+
+    const namedImports = (importStatement.match(NAMED_IMPORTS_REGEX) || [, ''])[1]
+      .split(',')
+      .map(name => name.trim())


Just making sure that this correctly returns ["a"] for both import {a as b} and import {a}.

FredKSchott · 2020-02-16T18:21:42Z

src/scan-imports.ts

+    }
+
+    const importStatement = code.substring(sStart, sEnd);
+    const dynamicImport = dynamicImportIndex > -1;


nit: it might be nice to push all of this logic into a seperate function as well, so that you could just do something like:

allImports.push(parseImport(importStatement));

FredKSchott · 2020-02-16T18:33:24Z

@alex-saunders yea, I think it's a bug in Node that that warning is still shown now that the behavior is out from behind a flag. Filed an issue here, lets see if its resolved quickly: nodejs/node#31819

alex-saunders · 2020-02-18T15:45:47Z

@FredKSchott I think I've addressed all your comments and I've added tests to cover the cases you mentioned.
Let me know if I missed anything.

stramel · 2020-02-18T16:29:44Z

Just curious when you guys say this is "much faster" and "definitely quicker", if we have a way to prove this or way to ensure we don't have regressions in our tooling performance? I'm pretty curious what kind of boost we are getting by switching to wasm based lexer

alex-saunders · 2020-02-18T16:45:52Z

@stramel es-module-lexer comes with benchmarking (https://github.com/guybedford/es-module-lexer#benchmarks) and the results are pretty impressive when compared with babel/full JS AST generation.
We could definitely run our own benchmarks but the results in es-module-lexer's README are pretty impressive as is.

FredKSchott · 2020-02-18T17:49:03Z

@stramel you're not wrong, there's definitely an "it's wasm so its faster" thing going on in my head 😜. Like @alex-saunders said, their README benchmarks are pretty promising, but that's a good reminder to confirm perf before merging

FredKSchott · 2020-02-18T17:52:21Z

@alex-saunders update on nodejs/node#31819: it seems to be their expected behavior that a warning is shown for now, but it looks like there's already a PR to remove the warning in an upcoming version. In the meantime @guybedford removed the export map, so we should be unblocked here / warning should now be gone in the latest version of es-module-lexer.

FredKSchott

LGTM! I left one or two nit comments but feel free to merge after resolving those.

I also checked it out and ran locally on www.pika.dev, which is a fairly complex install (using --include). Not only did everything on the site run as expected, but we saw a 1.5x speedup after switching from Babel to es-module-lexer! (/cc @guybedford thank you!)

Before (with tree-shaking via --optimize)

After (with tree-shaking via --optimize)

After (without --optimize)

FredKSchott · 2020-02-18T17:53:16Z

src/scan-imports.ts

@@ -76,47 +85,41 @@ function parseWebModuleSpecifier(specifier: string): null | string {
  return resolvedSpecifier;
 }

+function parseImport(code: string, imp: ImportSpecifier): null | InstallTarget {


Love how much it cleaned up the main function to move this out 👍

nit: parseImportStatement()

FredKSchott · 2020-02-18T17:54:38Z

src/scan-imports.ts

@@ -137,7 +140,8 @@ interface ScanImportsParams {
  exclude?: glob.IOptions['ignore'];
 }

-export function scanImports({include, exclude}: ScanImportsParams): InstallTarget[] {
+export async function scanImports({include, exclude}: ScanImportsParams): Promise<InstallTarget[]> {
+  await init;


nit: can we rename init to something like initESModuleLexer in the import statement?

FredKSchott · 2020-02-18T18:30:11Z

Just saw the test error, re-installing es-module-lexer to update the package version to latest should fix it.

drwpow · 2020-02-18T21:21:08Z

Way to go @alex-saunders! 🎉 This is awesome!

alex-saunders commented Feb 14, 2020

View reviewed changes

src/scan-imports.ts Outdated Show resolved Hide resolved

alex-saunders commented Feb 14, 2020

View reviewed changes

src/scan-imports.ts Outdated Show resolved Hide resolved

alex-saunders requested review from FredKSchott and drwpow and removed request for FredKSchott February 14, 2020 23:18

drwpow reviewed Feb 14, 2020

View reviewed changes

alex-saunders mentioned this pull request Feb 15, 2020

Update package exports guybedford/es-module-lexer#45

Merged

drwpow reviewed Feb 15, 2020

View reviewed changes

FredKSchott reviewed Feb 16, 2020

View reviewed changes

alex-saunders added 6 commits February 18, 2020 14:09

add es-module-lexer + types file

280123c

update getInstallTargetsForFile to work with es-module-lexer

3efd66c

cleanup

b425f2b

bump es-module-lexer version

fb7c705

review comments

cb04b47

add tests

8287312

alex-saunders force-pushed the es-module-lexer branch from b6b4ee1 to 8287312 Compare February 18, 2020 15:43

remove unnecessary file

c1535f8

FredKSchott approved these changes Feb 18, 2020

View reviewed changes

update es-module-lexer + minor improvements

3f46b18

alex-saunders merged commit fac12fc into FredKSchott:master Feb 18, 2020

alex-saunders deleted the es-module-lexer branch February 18, 2020 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace babel parse with es-module-lexer #208

Replace babel parse with es-module-lexer #208

alex-saunders commented Feb 14, 2020 •

edited

Loading

alex-saunders commented Feb 14, 2020

drwpow commented Feb 14, 2020

drwpow Feb 14, 2020

FredKSchott Feb 15, 2020

FredKSchott commented Feb 15, 2020

alex-saunders commented Feb 15, 2020

drwpow Feb 15, 2020

alex-saunders Feb 15, 2020

drwpow commented Feb 15, 2020

alex-saunders commented Feb 15, 2020

FredKSchott left a comment

FredKSchott Feb 16, 2020

FredKSchott Feb 16, 2020

FredKSchott commented Feb 16, 2020

alex-saunders commented Feb 18, 2020

stramel commented Feb 18, 2020

alex-saunders commented Feb 18, 2020 •

edited

Loading

FredKSchott commented Feb 18, 2020

FredKSchott commented Feb 18, 2020

FredKSchott left a comment •

edited

Loading

FredKSchott Feb 18, 2020

FredKSchott Feb 18, 2020

FredKSchott commented Feb 18, 2020

drwpow commented Feb 18, 2020

Replace babel parse with es-module-lexer #208

Replace babel parse with es-module-lexer #208

Conversation

alex-saunders commented Feb 14, 2020 • edited Loading

alex-saunders commented Feb 14, 2020

drwpow commented Feb 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott commented Feb 15, 2020

alex-saunders commented Feb 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow commented Feb 15, 2020

alex-saunders commented Feb 15, 2020

FredKSchott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott commented Feb 16, 2020

alex-saunders commented Feb 18, 2020

stramel commented Feb 18, 2020

alex-saunders commented Feb 18, 2020 • edited Loading

FredKSchott commented Feb 18, 2020

FredKSchott commented Feb 18, 2020

FredKSchott left a comment • edited Loading

Choose a reason for hiding this comment

Before (with tree-shaking via --optimize)

After (with tree-shaking via --optimize)

After (without --optimize)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott commented Feb 18, 2020

drwpow commented Feb 18, 2020

alex-saunders commented Feb 14, 2020 •

edited

Loading

alex-saunders commented Feb 18, 2020 •

edited

Loading

FredKSchott left a comment •

edited

Loading