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

Support 3rd party modules using both ESM and CommonJS #2946

Closed
wants to merge 3 commits into from

Conversation

buxxi
Copy link
Contributor

@buxxi buxxi commented Oct 14, 2022

The problem we had before with node-fetch migrating to an ESM got me thinking about what it would require to make 3rd party modules ESM. So I made this to test having the node_helper being either CommonJS or ESM.

If naming the file node_helper.mjs instead of node_helper.js it should load it as ESM instead.

This is just a draft, there are a few things left:

  • the module alias for node_helper isn't working, the module needs to specify the file with import NodeHelper from "../../js/node_helper.js"; which is annoying
  • I haven't done anything about the client side loading, but should be possible with <script type="module".../>, probably a bit harder to have the dual support of .mjs and .js without any configuration...

@buxxi buxxi changed the title Support 3rd party modules using ESM instead of CommonJS Support 3rd party modules using both ESM and CommonJS Oct 14, 2022
@rejas
Copy link
Collaborator

rejas commented Oct 14, 2022

Thanks for the brainstorming first of all.

But reading "the module alias for node_helper isn't working, the module needs to specify the file with import NodeHelper from "../../js/node_helper.js"; which is annoying" is a little understatement :-) Its quite impossible to get modules to use a new syntax without leaving a lot of old ones behind (and users possibly angry)

Using a more modern syntax is probably the way to go but maybe we need to make small steps (like your async work which looks like some stuff I try out too in my branches).

@buxxi
Copy link
Contributor Author

buxxi commented Oct 15, 2022

Could move most of the async stuff out to a separate PR to make the original intent smaller and possibly ignore my second point for now and only make this about the node_helpers. require() is sync which can only load CommonJS, import() is async which can load both, that's why I started with it and just felt the flow of continuing :)

This wasn't intended to break any of the existing modules (CommonJS modules should still load fine) and doing ESM modules will always be a bit different (no require(), import()/export(), no _dirname etc). I can't see node dropping CommonJS support without going the way of Python 2 & 3 and splitting up the community. The point is mainly about new modules and not the existing ones (sure I will migrate my own 3rd party modules). A JS developer coming from the frontend is probably already using ESM modules, Node documentation gives examples with import() and not require().

And I agree that the relative path is not an ok solution, just what I had to do for my test to actually work (as this is only a draft).

Will have to figure out why the tests segmentation faults too :)

rejas pushed a commit that referenced this pull request Oct 29, 2022
… next test (#2952)

When trying to debug why the tests broke for
#2946 I found that the tests
does not wait for the app to start and close. So if the startup isn't
blocking that would fail.

So I added a callback for `close()` too and converted them to promises
for the `startApplication()` and `stopApplication()` and updated all the
e2e tests to await both. Will try to refactor all these callbacks to
promises in a later PR.
@buxxi
Copy link
Contributor Author

buxxi commented Nov 4, 2022

Since I failed to solve the prerequisite PR #2962 I will close this for now.

@buxxi buxxi closed this Nov 4, 2022
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.

None yet

2 participants