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.

Sign up for GitHub

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

Jump to bottom

feat(config): add support for ES modules #2685

Closed
wants to merge 1 commit into from
Closed

feat(config): add support for ES modules #2685

wants to merge 1 commit into from

Conversation

wesleycho
Copy link
Member

  • Add support for ES modules via esModule property in file config object

This should address #2684.

@wesleycho
Copy link
Member Author

@appsforartists can you check this change out and see if this works for you? I think this should be enough to support module loading.

@wesleycho wesleycho force-pushed the feat/esmodule branch 2 times, most recently from a811b70 to 8d3fa7e Compare May 4, 2017 15:56
appsforartists
appsforartists reviewed May 4, 2017
View reviewed changes
lib/middleware/karma.js Outdated
@@ -178,6 +178,7 @@ var createKarmaMiddleware = function (
var file = files.included[i]
var filePath = file.path
var fileExt = path.extname(filePath)
var esModule = fileExt === '.js' && file.esModule
Copy link

Choose a reason for hiding this comment

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

.endsWith('js') might be better - there's talk of changing the extension to 'mjs'

@wesleycho wesleycho force-pushed the feat/esmodule branch 2 times, most recently from 0fe4f20 to da2ef8d Compare May 4, 2017 16:39
@dignifiedquire
Copy link
Member

This should have at least one e2e test, making sure this actually works as expected.

@wesleycho
Copy link
Member Author

Where would you recommend going about implementing a test for this? Is it the .feature files in test/e2e?

@dignifiedquire
Copy link
Member

Where would you recommend going about implementing a test for this? Is it the .feature files in test/e2e?

Yes, something like module-types.feature, testing the different module types and that they are executed as expected

@wesleycho wesleycho force-pushed the feat/esmodule branch 5 times, most recently from af6e64d to 7cd386a Compare May 4, 2017 18:51
@wesleycho
Copy link
Member Author

Question here - I don't believe PhantomJS supports ES modules, so how would such a test run in this scenario? It looks like we don't have Chrome available...is there a way we can use a modern browser for this test?

@dignifiedquire
Copy link
Member

Question here - I don't believe PhantomJS supports ES modules, so how would such a test run in this scenario? It looks like we don't have Chrome available...is there a way we can use a modern browser for this test?

Firefox should be always available

@wesleycho wesleycho force-pushed the feat/esmodule branch 4 times, most recently from 9f4dd6b to 6ce97e6 Compare May 4, 2017 22:25
@wesleycho
feat(config): add support for ES modules
fd59093 
- Add support for ES modules via `esModule` property in file config object
@appsforartists
Copy link

@wesleycho I updated your test to ensure that it correctly tests that modules and relative imports work:

wesleycho#1

@Florian-R
Copy link

On a more practical note, how would you use this today? Unless I miss something, you can't change chrome:flags or Firefox's about:config with their respective launchers.

@wesleycho
Copy link
Member Author

I'm confused by your question - browsers support module loading via <script type="module" src="..."></script>. Note that this is different from the dynamic import proposal, and solely based on the script tag.

This PR is about adding support for loading scripts via this mechanism for browsers that support it based on the user's preferred setup. It will not work for browsers that do not support this, but for those situations, one should not have a karma config that sets this flag to true anyway.

@Florian-R
Copy link

My apologies, I should've try to be more clearer.

To quote the article linked in #2684

ES modules are starting to land in browsers! They're in…

Safari 10.1.
Chrome Canary 60 – behind the Experimental Web Platform flag in chrome:flags.
Firefox 54 – behind the dom.moduleScripts.enabled setting in about:config.
Edge 15 – behind the Experimental JavaScript Features setting in about:flags.

For most browsers, this works only by setting an experimental flag. On a dev machine, this is definitively doable, but I have a hard time to understand how to make this works in a CI hence my question.

Hope it's clearer. (and thanks for doing that work, it's definitely cool to be able to poke with this).

@wesleycho
Copy link
Member Author

Ah I see - so this is more about the future. When they move out of experimental, then they will be supported in truth.

@appsforartists
Copy link

@wesleycho You gonna merge wesleycho#1?

@jimsimon
Copy link

I'm running into this issue on a project I'm working on. Any chance this is going to be merged and released soon?

@jimsimon
Copy link

jimsimon commented Aug 1, 2017

@Florian-R FWIW, there are definitely options for enabling flags in a CI environment. It can be done in Karma (at least for Chrome) with a custom launcher. Many enterprises also control their browser configurations for internal users, so it's entirely possible that module loading via <script type="module" src="..."></script> could be used in production today (especially since Chrome 60 is officially released now).

@abdel
Copy link

abdel commented Sep 15, 2017

@wesleycho any update on this PR?

@mdrobny
Copy link

mdrobny commented Sep 29, 2017

What is the status of this PR? Es modules have better and better support and I would really like to use it for my tests since I can easily define my tests target browser.
There is Chrome 61, Safari 10.1 without flags needed and even iOS and Android browsers

@appsforartists
Copy link

@dignifiedquire Looks like @wesleycho hasn't been on GitHub in a while. Wanna take this one over?

I had fixed his tests in wesleycho#1, but doesn't look like he's seen that either. Would be nice if we merged both PRs.

@wesleycho
Copy link
Member Author

I am sadly not able to make any updates currently due to having switched jobs and needing approval from various parties before I am allowed to do any open source work again. If someone wants to take over for this, feel free!

@appsforartists
Copy link

@wesleycho - can you please merge the PR I made to your PR, so we at least have a single artifact to take over? 😃

@wesleycho
Copy link
Member Author

I can't do so - even that I am not allowed to do currently. It would be best to file a new PR to this project.

@appsforartists
Copy link

OK, I'll make a new PR here.

@appsforartists appsforartists mentioned this pull request Sep 29, 2017
Closed
@appsforartists
Copy link

Moved to #2834. This issue can be closed now.

Thanks for your help, @wesleycho. Sorry you can't finish it. 😢

@johnjbarton
Copy link
Contributor

Thanks for the effort, closing in favor of #2834

@johnjbarton
Copy link
Contributor

Fixed by #3072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@appsforartists appsforartists appsforartists left review comments

@dignifiedquire dignifiedquire dignifiedquire approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@wesleycho @dignifiedquire @appsforartists @Florian-R @jimsimon @abdel @mdrobny @johnjbarton