Skip to content

Conversation

@RReverser
Copy link
Collaborator

A bit crude, since Terser doesn't have a dedicated dynamic import AST node type, but verified that it works.

Fixes #11303 (now for real).

A bit crude, since Terser doesn't have a dedicated dynamic import AST node type, but verified that it works.

Fixes emscripten-core#11303 (now for real).
@RReverser RReverser requested review from kripken and sbc100 September 29, 2020 12:22
@curiousdannii
Copy link
Contributor

@RReverser
Copy link
Collaborator Author

No, our version has already diverged, plus as mentioned in the description this is a limited support just for our use-case; proper upstream support would be much more involved and different from this PR.

@RReverser
Copy link
Collaborator Author

Tests passed!

@sbc100
Copy link
Collaborator

sbc100 commented Sep 29, 2020

Should there be a test case as part of this change?

@RReverser
Copy link
Collaborator Author

Mayyyybe 😅

@RReverser
Copy link
Collaborator Author

I guess I was somewhat discouraged by your TODO / comment here:

# TODO(sbc): Test that this is actually runnable. We currently don't have
# any tests for EXPORT_ES6 but once we do this should be enabled.
# self.assertContained('hello, world!', self.run_js('hello_world.mjs'))

But I guess I could add a test that simply verifies this combination of flags compiles at all.

@RReverser
Copy link
Collaborator Author

(working on it)

@sbc100
Copy link
Collaborator

sbc100 commented Sep 29, 2020

Sounds good. Just added a compile-only test would be acceptable for now.. given that TODO.

If you are interested in this area perhaps we might want to find a way to do actual testing of the generated ES6 modules.

@RReverser
Copy link
Collaborator Author

Added a test and verified that it fails without the fix and works with the fix.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but I don't know the terser code.. @azakai can you take a look?

@RReverser
Copy link
Collaborator Author

lgtm but I don't know the terser code

FWIW I kind of know that file because I implemented quite a few parts of it back in 2014 when it was still UglifyJS, but an extra pair of eyes is surely welcome 😅

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok sgtm

@sbc100 sbc100 merged commit 47a1c28 into emscripten-core:master Sep 29, 2020
@RReverser RReverser deleted the terser-dynamic-import branch September 29, 2020 18:37
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.

Cannot use EXPORT_ES6=1 and USE_PTHREADS=1 at the same time

3 participants