Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

feat: ESM modules support #1323

Closed
wants to merge 6 commits into from
Closed

feat: ESM modules support #1323

wants to merge 6 commits into from

Conversation

robertsLando
Copy link
Contributor

@robertsLando robertsLando commented Sep 30, 2021

Ref #1291

To run test: FLAVOR=test-00-esm npm test

Copy link
Contributor

@jesec jesec left a comment

Choose a reason for hiding this comment

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

There is a (huge) binary in the commit.

@robertsLando
Copy link
Contributor Author

robertsLando commented Sep 30, 2021

Sorry the test has fails and i forgot to delete it, when it works the vacoom automatically removes it.

Just to be clear tests will not pass as this is not a fix to #1291 but it just reproduce the problem, I tried to fix it without success. Do you have any clue what's not working?

@jesec
Copy link
Contributor

jesec commented Oct 1, 2021

Sorry the test has fails and i forgot to delete it, when it works the vacoom automatically removes it.

Just to be clear tests will not pass as this is not a fix to #1291 but it just reproduce the problem, I tried to fix it without success. Do you have any clue what's not working?

#782

We have no support for ES6 modules yet. The walker can't parse the dependencies and include them in the payload.

@robertsLando
Copy link
Contributor Author

robertsLando commented Oct 1, 2021

The strange thing is that the detector actually identifies import statements:

was = visitorImport(node);

Also in latest ncc updates now it outputs esm modules so it's not a solution at all, see this. I think we need to add a support to esm modules as with node 14+ them are supported. If you could point me on the changes needed I could try to fix this.

@robertsLando
Copy link
Contributor Author

robertsLando commented Oct 6, 2021

@jesec news here? Are you sure the problem is in walker so?

@robertsLando
Copy link
Contributor Author

robertsLando commented Oct 6, 2021

Ok I answer my self. The problem is that actually resolve package cannot handle esm modules yet: browserify/resolve#222

A possible fix could be try using https://www.npmjs.com/package/enhanced-resolve

We could also consider usign https://github.com/bytenode/bytenode as fabricator

@robertsLando robertsLando changed the title test: add esm module test feat: ESM modules support Oct 6, 2021
@robertsLando robertsLando linked an issue Oct 6, 2021 that may be closed by this pull request
@robertsLando
Copy link
Contributor Author

This could be a starting point: vitejs/vite#5593

@jesec
Copy link
Contributor

jesec commented Nov 10, 2021

Our use case is considerably more complicated. enhanced-resolve is nowhere near to a drop-in replacement for resolve in our case.

We would have to study the APIs of the enhanced-resolve and rewrite our logics.

@robertsLando
Copy link
Contributor Author

robertsLando commented Nov 10, 2021

We would have to study the APIs of the enhanced-resolve and rewrite our logics.

I agree, in my tests I was just tring to see what happened using that as a separete tool then the plan it's to completely replace resolve. I think this issue should be the top in our todos (giving the amount of related open bugs), do you want to do some tries? Test is already setted up

@hcldan
Copy link

hcldan commented Jan 21, 2022

I have been using es6 in my projects, but before I run pkg I transpile them to commonjs. This seems to work well. It would be great if this support landed natively or if you just build in the transpiling part so I didn't have to.

@robertsLando
Copy link
Contributor Author

I transpile them to commonjs

Do you use webpack for this?

@hcldan
Copy link

hcldan commented Jan 24, 2022

rollup worked really well, as I don't need the features of webpack, I just need my modules to be cjs in the end.

package.json

{
  "scripts": {
     "build": "npm-run-all build:*",
     "build:1": "rimraf dist",
     "build:2": "mkdirp dist",
     "build:3": "rollup index.js -o dist/built.js --format cjs",
     "build:4": "pkg dist/built.js --targets linux,win --out-path dist"
  }
}

@hcldan
Copy link

hcldan commented Jan 24, 2022

sourcemaps are a pain... so native support would be fantastic

@RUGMJ
Copy link

RUGMJ commented Mar 3, 2022

are there any updates or workarounds for this? I really need to distrabute my package ASAP

@kamack38
Copy link

kamack38 commented Mar 3, 2022

are there any updates or workarounds for this? I really need to distrabute my package ASAP

You can use caxa, because they are supporting ESM.

@hcldan
Copy link

hcldan commented Mar 4, 2022

I've commented above for a decent workaround that I'm using

rollup can be configured to transpile and bundle all es modules in a file and defer all commonjs modules to the default require.

  "scripts": {
    "test": "npm-run-all test:*",
    "test:1": "eslint",
    "test:2": "NODE_OPTIONS=--experimental-vm-modules npx jest",
    "start": "node .",
    "build": "npm-run-all build:*",
    "build:1": "rimraf dist",
    "build:2": "mkdirp dist",
    "build:4": "rollup --config rollup.config.js",
    "build:5": "pkg dist/nwsp.js --targets linux,win --out-path dist"
  },

rollup.config.js

import nodeResolve from 'rollup-plugin-node-resolve';

export default {
  input: 'index.js',
  output: {
    file: 'dist/nwsp.js',
    format: 'cjs',
  },
  plugins: [
    nodeResolve({
      modulesOnly: true,
    }),
  ],
};

There are tons of options, look them over. This gives you one big file with all es5 turned into commonjs.
Only big drawbacks here are complexity (it would be nice if this all just worked) and sourcemap support.
Rollup can output sourcemaps (even inline) but I cant get stacks reported by node to use that info.

@gaby
Copy link

gaby commented Apr 22, 2022

Any updates on this feature?

@github-actions
Copy link

This pull-request is stale because it has been open 90 days with no activity. Remove the stale label or comment or this will be closed in 5 days. To ignore this pull-request entirely you can add the no-stale label

@github-actions github-actions bot added the Stale label Sep 19, 2022
@gaby
Copy link

gaby commented Sep 19, 2022

Any reason why stale bot is trying to close PR's?

@CleyFaye
Copy link

Any reason why stale bot is trying to close PR's?

I'd say it's

because it has been open 90 days with no activity

@github-actions github-actions bot removed the Stale label Sep 20, 2022
@angrymouse
Copy link

Any updates on it?

@edosrecki
Copy link

Any news at all?

@cspotcode
Copy link

I think there is another issue here: even if the files are bundled into the virtual filesystem, it cannot be executed. I'm guessing node's ESM resolver does not use the patched filesystem.

For example, you can bundle an ESM file alongside an CJS file, using the CJS file as entrypoint. Then run stuff in the CJS file that looks vaguely like this:

// entrypoint.cjs:
// Prove that the file exists in VFS
console.log(fs.readdirSync('/snapshot'));
console.log(fs.readFileSync('/snapshot/bin.mjs'));
// But import fails
import('/snapshot/bin.mjs');

Gives a cryptic error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/snapshot/bin.mjs' imported from /snapshot/entrypoint.cjs
Did you mean to import /snapshot/bin.mjs?

Looking closely at that error, it says i cannot find the module, then it suggests exactly the same path. I bet the suggestion comes from different code which is using the VFS.

@robertsLando
Copy link
Contributor Author

Unfortunately I have no clue about how to actually implement such feature, I would need some support on this

@Haringat
Copy link

Haringat commented Apr 6, 2023

@robertsLando Perhaps you could create a custom loader that uses the virtual filesystem.
Here are some example implementations from the node docs: https://nodejs.org/dist/latest-v19.x/docs/api/esm.html#loaders

@robertsLando
Copy link
Contributor Author

@Haringat Thanks for your suggestion, BTW I don't think that would solve the issue, right now the problem is we don't have a way to resolve the modules used with esm, enhanced resolve should solve this issue but I have no clue how to make it work

@Haringat
Copy link

Haringat commented Apr 7, 2023

@robertsLando But with the loader you can specify a custom resolve hook to do just that. It gets the URL it is supposed to resolve and the URL of the importing module.

@robertsLando
Copy link
Contributor Author

It would not work for dynamically loaded modules I think? I mean an async import for example will not be catched by it on startup

@Haringat
Copy link

Haringat commented Apr 7, 2023

@robertsLando I just tested it with an HTTP/HTTPS loader and it was also invoked for a dynamic import.

However, I have absolutely no clue how createRequire would work in such a case. That would be an interesting test, though.

Edit: I tried it with that HTTP loader and a js file on localhost, but node.js wisely blocks imports from node builtin modules with the error "import of 'node:module' by http://127.0.0.1:8080/index.mjs is not supported: only relative and absolute specifiers are supported."
I guess that would not be a problem in your case, though, as you could use the file protocol for your vfs and have node not even realize what is going on.

@robertsLando
Copy link
Contributor Author

robertsLando commented Apr 8, 2023

The problem is different here, we actually don't run a file with node to detect it's dependencies, we start from the entrypoint file and from that we recursively resolve all modules using "resolve" npm module, to detect them we simply use babel to find require/import statements in the files. Your suggestion so would not work in this case at is requires to run the entrypoint file, also there is no way to know how to trigger dynamic import in that way.

@github-actions
Copy link

github-actions bot commented Jul 8, 2023

This pull-request is stale because it has been open 90 days with no activity. Remove the stale label or comment or this will be closed in 5 days. To ignore this pull-request entirely you can add the no-stale label

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

This pull-request is stale because it has been open 90 days with no activity. Remove the stale label or comment or this will be closed in 5 days. To ignore this pull-request entirely you can add the no-stale label

@github-actions github-actions bot added the Stale label Oct 9, 2023
@rightaway rightaway mentioned this pull request Oct 10, 2023
@github-actions
Copy link

This pull-request is now closed due to inactivity, you can of course reopen or reference this pull-request if you see fit.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ES modules not supported