Investigate es-module-lexer

[FredKSchott]

Just found https://github.com/guybedford/es-module-lexer
Seems like it could be a drop-in replacement for our import scanner. Also powered by WASM meaning it's wicked fast. Would like to investigate further and possibly integrate.

/cc @DangoDev

[alex-saunders]

@FredKSchott do you know if there is anyone actively working on this? I'd love to investigate this but don't want to duplicate any effort that is already being used to implement this.

[drwpow]

I think it sounds like a cool idea but I have not started on anything (can’t speak for @FredKSchott tho). Go for it if you’re interested!

…

On Jan 13, 2020, at 14:37, Alex Saunders ***@***.***> wrote:  @FredKSchott do you know if there is anyone actively working on this? I'd love to investigate this but don't want to duplicate any effort that is already being used to implement this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[FredKSchott]

Nope, and I'd love the help! We essentially would want to replace the meat of this function: https://github.com/pikapkg/snowpack/blob/d96f01020af15e81e6c7d9455e3751f8f516fb55/src/scan-imports.ts#L70-L118 with es-module-lexer while keeping the same function args & outputs. @alex-saunders does that make sense?

[alex-saunders]

Sounds great, I can certainly have a go! It's the end of the day where I am but I'll pick this up tomorrow and try to get a PR ready in the next couple days if I can

Thanks for pointing me in the right direction @FredKSchott 😄

Would it make sense to assign this issue to me? Just incase someone misses this conversation and starts working on the same thing?

[FredKSchott]

done!

[FredKSchott]

@alex-saunders I hope you don't mind, but I went ahead and investigated this a bit deeper and it turns out we're blocked on this missing feature: guybedford/es-module-lexer#36

Basically, the library can parse imports but only returns an array of import specifiers (the URL bit). Unfortunately, we need to parse the entire statement to see what exact imports are used off each module. That info is required for tree-shaking.

[alex-saunders]

@FredKSchott no problem at all, I had actually encountered the same issue (hence why my PR was taking a bit longer than anticipated!). I was trying to hack around it but wasn't having much luck so fixing this within the actual es-module-lexer package sounds like a smart plan.
I'll follow the es-module-lexer issue and help out there if I can so that we can get this working

[FredKSchott]

SGTM! Thanks again for taking a look at this. Moving the issue back to our Pika discussion board until its unblocked / we can re-prioritize.

[FredKSchott]

Okay, guybedford/es-module-lexer#41 was merged and ex-module-lexer now returns the full import statement, which includes named exports! @alex-saunders are you still interested in contributing this improvement? It would be much appreciated if you can!

[alex-saunders]

Absolutely @FredKSchott I'm happy to take this on if it hasn't already been picked up!

[drwpow]

@alex-saunders go for it! Please feel free to reach out with any questions you have!

Probably one thing that may help to explain what scan-imports.ts is doing is to focus on this type right here:

export type InstallTarget = {
  specifier: string;
  all: boolean;
  default: boolean;
  namespace: boolean;
  named: string[];
};

This object is the key to understanding imports. As long as an array of these objects is generated, the rest of Snowpack should do the rest. So for the following imports, this is what should be generated by the getInstallTargetsForFile() method:

// 1. Default import
import React from '/web_modules/react.js'

installTargets.push({
  specifier: '/web_modules/react.js',
  all: false, // this should be tree-shaken
  default: true, // this is a default import
  namespace: false,
  named: []
})

// 2. Named import
import { useState, useEffect } from '/web_modules/react.js'

installTargets.push({
  specifier: '/web_modules/react.js',
  all: false, // this should be tree-shaken
  default: false,
  namespace: true, // this is a namespaced import
  named: ['useState', 'useEffect']
})

// 3. Dynamic import
const React = import('https://cdn.pika.dev/@pika/react');

installTargets.push({
  specifier: 'https://cdn.pika.dev/@pika/react',
  all: true, // don’t try and tree-shake this; we don’t know what’s needed b/c it’s runtime
  default: false, // it’s lazy, so dunno if it’s default
  namespace: false, // it’s lazy, so dunno if namespaced
  named: []
})

Of course, there’s a little more going on in that getInstallTargetsForFile() method, but not much more. This is the heart of the scanner right here.

---

For testing, we do more E2E tests in __tests__/integration which I prefer, but those can get a bit snapshot-y (meaning that we only test if they match a large, generated folder of files, but because the expected-install folders are large and complicated it’s not clear if absolutely 100% is correct, and updating them to fix one thing may break another hidden thing).

So all that to say, maybe breaking up the include test into testing each different type of module separately may also be a good sub-goal for this (and I suggest it to only make your work easier because these 3 major types of imports are handled slightly-differently).

[alex-saunders]

Wow, thanks so much for the detailed help @DangoDev . That's definitely enough info for me to go on and try to implement this.

Breaking up the tests sounds like a smart plan and definitely something that will help development so I'll aim to do this.

Thanks again and I'll let you know how I get on!

[alex-saunders]

@DangoDev @FredKSchott Just keeping y'all up to date - I'm working on this now. I was a bit preoccupied with the --stat PR but this is my focus now.

[FredKSchott]

Awesome! Let me know if I can help at all.

PS: Hoping to do a new release tomorrow with --stat!