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

Add ES6 exports #6483

Merged
merged 1 commit into from
May 30, 2018
Merged

Conversation

curiousdannii
Copy link
Contributor

@curiousdannii curiousdannii commented Apr 28, 2018

Fixes #6284

The code change is very simple. But I'm not sure about tests.

What version of Node is bundled in emsdk?
Can I just make a simple test that checks importing the built module works, or does it need more rigorous testing?

Node requires ES6 modules to have the extension .mjs. Should this mode automatically export files as a.out.mjs instead of .js? And it should support -o out.mjs. Maybe that could even automatically turn on EXPORT_ES6 if we wanted to be clever!

@kripken
Copy link
Member

kripken commented Apr 30, 2018

Most people just use the already-installed node, I think. Looking in our docs, we don't mention a minimum version currently. Would this require a recent version?

@curiousdannii
Copy link
Contributor Author

curiousdannii commented Apr 30, 2018

From 8.5, but it's behind a flag which would make testing more difficult. And actually it wouldn't be needed in a emsdk, just the testing environment.

@kripken
Copy link
Member

kripken commented May 2, 2018

I see. Well, if it's behind a flag I guess we can wait on testing it. Once it's enabled in a stable node release we can make sure that's on the bots and add a test. How about writing up the test now, though (in other, I guess) and leaving it disabled, so in the future we just need to enable it?

@kripken
Copy link
Member

kripken commented May 17, 2018

Should we land this? We can figure out testing later I guess.

@curiousdannii
Copy link
Contributor Author

Sorry, I was planning to write tests, but haven't had much time.

@xtuc
Copy link

xtuc commented May 17, 2018

I can also review it and actually test it as well.

@xtuc
Copy link

xtuc commented May 17, 2018

I'm testing with: ./emcc.py -s MODULARIZE=1 -s EXPORT_ES6=1 -s WASM=1 t.c.

I noticed that import are:

(import "env" "memory" )
(import "global" "NaN" (global f64))
;; ...

In order to work with JavaScript bundlers (Webpack and Rollup atm) and probably Node, the imports module should be a local file. The glue needs to use a named export then.

It seems to be a bigger change to the glue, but I think that would be worth it in the long run.

Node requires ES6 modules to have the extension .mjs

I think it's fine, it will also wok with .js for now.

@kripken
Copy link
Member

kripken commented May 30, 2018

Ok, thanks @curiousdannii and @xtuc , let's merge this in for now. @curiousdannii, if you get a chance to add tests later that would be great!

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

Successfully merging this pull request may close these issues.

3 participants