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

Full import/export statement tracking #36

Closed
guybedford opened this issue Sep 6, 2019 · 14 comments
Closed

Full import/export statement tracking #36

guybedford opened this issue Sep 6, 2019 · 14 comments

Comments

@guybedford
Copy link
Owner

If we want to support hosting of import/export statements (needed for TLA), we should extend the data associated with imports and exports to include two new properties - ss and se for statement start and statement end.

In the case of statements like export function x () {} we can actually return -1 for se since a hoisting operation would just need to remove the export keyword and move that to the hoisted section.

@FredKSchott
Copy link
Contributor

Hey Guy, we're really excited about using this lib as a replacement for our custom Babel-based import scanner in Snowpack. Unfortunately, full import statement tracking is required for our use-case. More info here, if you're curious: FredKSchott/snowpack#146

I'm happy to help implement this if it's straightforward and you're interested (/cc @alex-saunders who may be interested as well).

@guybedford
Copy link
Owner Author

@FredKSchott supporting full import binding tracking could certainly be possible, and shouldn't have any technical blockers. This project tries to keep the code footprint down as much as possible to avoid unnecessary costs for es-module-shims, so depending on how much this increases the final file size it could even be sensible to fork to get exactly the API you need, if it turns out it adds more than a few hundred bytes to the output size.

@FredKSchott
Copy link
Contributor

Okay, thanks. I'll see how far I can get into this then. Would love your guidance on it, it's been ages since i've done C development like this.

Are you alright if we tackle imports first? Grabbing the whole "export" statement feels like a very different can of worms since the variable declaration can be any value (vs. import statements being much easier to statically analyze). Not sure if I have the resources to do both.

@FredKSchott
Copy link
Contributor

FredKSchott commented Jan 23, 2020

My first approach to parsing the import statement would be:

  • "statement end" is always the same of the current "end" value +/1 0 for import.meta, +1 for regular imports (for the end quote), and +2 for dynamic imports (for the end paren and quote). We don't need any new data to track this, just calculate it inside of addImport() based on dynamic, and then add it to the return object under se.
  • We need to pass startPos from tryParseImportStatement() as a new argument into addImport(). We then add it to the return object under ss. No new logic needed otherwise.
  • export from ... just means that we need to add a startPos variable to tryParseExportStatement() as well, so that we can pass that through to addImport().

@guybedford Does this approach make sense? Overall this only adds one startPos pointer to the export parser, 1 argument to addImport(), and then one block of logic to calculate statement end (I got it naively in a 5 line if-else, but assuming we might be able to simplify that even further). Seems reasonable to me.

@guybedford
Copy link
Owner Author

@FredKSchott I initially thought you were planning to seek emission of each of the specifiers in the import statement - but if getting the parse result of the entire statement itself (ie to then put through a specific parser on the statement boundary to get the perf benefits) is your goal here then that definitely sounds like a worthwhile approach to me.

Implementing such a statement end for both import and export via a .se or similar sounds great, with it being skipped for the dynamic import forms.

The build script at https://github.com/guybedford/es-module-lexer/blob/master/Makefile#L6 currently assumes as WASI location which can be somewhat of a pain to configure, but that's the main hurdle to getting it set up I believe. Ideally this whole project could be ported to Zig...

Just let me know if you have any issues getting started.

@FredKSchott
Copy link
Contributor

Awesome, glad that approach aligns more with your vision 👍

Okay, I'll push up a PR to follow the plan outlined above. That's a good point that you don't actually need "statement start/end" for dynamic imports or import.meta. I'll follow your advice and just skip it on those, only outputting for normal import/export form.

@FredKSchott
Copy link
Contributor

Closing this loop, I think this is safe to close?

@guybedford
Copy link
Owner Author

There are still some edge cases of full export statement tracking that are missing I think, since we only handled this for import and reexport statements. Cases like full function statement exports still need to be tracked to work towards top-level await rewriting needed by es module lexer.

@diervo
Copy link

diervo commented May 15, 2020

@guybedford Is there a PR/issue I can follow/help where the export statement location is implemented discussed? For our use case we do need to know the location of the exports statements too.

@guybedford
Copy link
Owner Author

guybedford commented May 16, 2020

@diervo hey, well this is it :)

We need to handle all export declaration forms fully in ouputting their location information.

So the following forms need to be detected:

  1. Function and class exports:
export function name () {
}

This is a high priority for me as it will allow us to support top-level await in es-module-shims.

Including async / generator variations. This should be straightforward as we can find the end of the param list function itself by just matching the brackets and braces. It is still a bit of work to track though in comparison to the simple export function name (... yah we good that we do right now!

Classes should be straightforward based on the same sort of principle. The expression in the SuperExpression position can be skipped easily since we know that the next opening brace at the same overall brace and bracket depth has to be the class, just needs a bit of thought.

Update: Thinking about this more - I'm not 100% sure... could be worth double-checking the language grammar.

  1. Variable declarations
export var p = value, q = hmm;

A similar approach to class super expressions might actually work for variable declarations - where a , at the same bracket and brace depth must be the next declaration allowing us to still avoid the expression parsing.

It would be worth checking against the language grammar this can definitely be relied on as we currently describe this as a limitation (https://github.com/guybedford/es-module-lexer#limitations).

  1. Default export

With function exports and class exports, the full default export should then be straightforward based on combining the above.

It will be quite a bit of work yet, and will probably increase the codebase by quite a little, but I am keen to go down these paths.

I definitely want to get to it, I just don't personally have the bandwidth for it in the next few months. Are you interested in contributing or have someone there contribute? Let me know your thoughts.

@diervo
Copy link

diervo commented May 16, 2020

My C its a bit rusty so it will take me more time that I can manage right now, but maybe @pmdartus might take it as a challenge? 😄

@bjufre
Copy link

bjufre commented Jun 1, 2021

@diervo is there any updates/progress on this?

@diervo
Copy link

diervo commented Jun 2, 2021

Unfortunately I wasn't able to get this prioritized on any of my teams at Salesforce so far, and I'm on paternity leave right now so on my end it will take a while to be able to help in the short term...

@guybedford
Copy link
Owner Author

Closing to track in individual issues instead as many issues cover this topic, we seem to be slowly making progress in improving the granularity over time.

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

No branches or pull requests

4 participants