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

Provide some way to annotate exports? #34

Closed
evanw opened this issue Jan 13, 2021 · 7 comments
Closed

Provide some way to annotate exports? #34

evanw opened this issue Jan 13, 2021 · 7 comments

Comments

@evanw
Copy link

evanw commented Jan 13, 2021

I'm trying to figure out what to do about evanw/esbuild#668, which is an issue someone logged with esbuild because esbuild's export syntax doesn't exactly match the magic syntax forms used by this library.

I'm using a for-loop to generate export getters since it minifies better, but to work with this library I would presumably have to write out a long verbose list of Object.defineProperty() calls instead. This is undesirable both for minification reasons and because it would require me to support another code path, which is more overhead for me in terms of code complexity and would lead to a bigger test matrix.

Would you consider adding some way to annotate the exports that are available in a given file without having to rewrite the JavaScript in the file? A comment with something like //# cjsExports="someName","someOtherName",... would work, for example. I'm asking because this seems like the most straightforward solution. This library is trying to get a list of exports and instead of trying to magically infer the exports from the code, it could just read them out directly from a list.

@guybedford
Copy link
Collaborator

We did discuss syntax approaches like this, but it seemed like in most cases the advisable approach was to use ESM wrappers or ESM modules directly for these executions since if Node.js supports this feature it would support ESM anyway.

We can make changes to this processing, but backporting is not necessarily a given at this point so changing things here would result in spotty support for 12 LTS, whereas right now there is comprehensive backwards compat on all Node.js modules semantics paths so it can be reliable.

//cc @GeoffreyBooth @MylesBorins @bmeck @jkrems

@MylesBorins
Copy link
Member

Yeah, unfortunately Node.js v12 is now in maintenance so the bar to make changes is going to be pretty hard... especially ones that are not backwards compatible. Not impossible, but very very hard to do.

@GeoffreyBooth
Copy link
Member

Would you consider adding some way to annotate the exports that are available in a given file without having to rewrite the JavaScript in the file? A comment with something like //# cjsExports="someName","someOtherName",... would work

If esbuild could output such a comment, wouldn’t it also therefore be able to output a CommonJS module that cjs-module-lexer could successfully statically analyze? That seems like the most straightforward solution, as then nothing would need to change on Node’s side and it would work back to Node 12.

@evanw
Copy link
Author

evanw commented Jan 18, 2021

If esbuild could output such a comment, wouldn’t it also therefore be able to output a CommonJS module that cjs-module-lexer could successfully statically analyze?

Yes it could. I addressed this in my original post:

This is undesirable both for minification reasons and because it would require me to support another code path, which is more overhead for me in terms of code complexity and would lead to a bigger test matrix.

It's fine if your answer is "no". The decision is up to you since it's your project. Just thought I'd ask first to see if there was a way to avoid the complexity of having to generate this magic syntax.

@guybedford
Copy link
Collaborator

@evanw it might be possible to write out an abbreviated form that basically fools the lexer, to avoid those calls being the full exact defineProperty statements. In a way this might effectively be like what you were asking about.

The lexer doesn't have any concept of execution flow at all, so a non-executed function that looks like it defines exports would be supported.

Here's one way of doing that:

// fake exports to inform lexer
(() => exports.a = exports.b = exports.c = void);

// actual exports definition function
defineExports(exports);

That is then something that is quite close to the syntax you are after that will work all the way back to Node.js 12 as well.

I'd still recommend simply outputting an analyzable form of exports definitions if you can and that would still be preferable. But if you can't then the above can be an option.

@guybedford
Copy link
Collaborator

guybedford commented Jan 18, 2021

I guess the remaining risk is minifiers removing these unused functions... perhaps there's a way to get around that as well?

That said, the same likely applies to any "syntax approach" even the one asked.

@guybedford
Copy link
Collaborator

It seems like #46 or similar will be the approach for now.

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