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

Re-introduce CommonJS support to SDK #686

Closed
MadLittleMods opened this issue Feb 18, 2022 · 5 comments · Fixed by #693
Closed

Re-introduce CommonJS support to SDK #686

MadLittleMods opened this issue Feb 18, 2022 · 5 comments · Fixed by #693
Assignees
Labels

Comments

@MadLittleMods
Copy link
Contributor

MadLittleMods commented Feb 18, 2022

It's not quite clear what the original problem is or how to reproduce but https://github.com/vector-im/hydrogen-web/pull/633/files removed CommonJS exports from the package.json to workaround a Vite bug.

With https://github.com/matrix-org/matrix-public-archive, I'm requiring hydrogen-view-sdk in Node.js. I'm currently working around the CommonJS support by just manually updating package.json with the exports again.

package.json

{
  "main": "./hydrogen.cjs.js",
  "exports": {
      ".": {
          "import": "./lib-build/hydrogen.es.js",
          "require": "./lib-build/hydrogen.cjs.js"
      },
      "./paths/vite": "./paths/vite.js",
      "./style.css": "./style.css",
      "./assets/*": "./asset-build/assets/*"
  }
}
@bwindels
Copy link
Contributor

bwindels commented Feb 21, 2022

Noted, I'm surprised though you can't use hydrogen-view-sdk as an ES module though from your commonjs project with node. Are you using an older version of node?

@MadLittleMods
Copy link
Contributor Author

Node.js v16.14.0

If you try to require('hydrogen-view-sdk') as an ES module, Node.js throws:

Error [ERR_REQUIRE_ESM]: require() of ES Module C:\Users\MLM\Documents\GitHub\element\hydrogen-web\target\hydrogen.es.js from C:\Users\MLM\Documents\GitHub\element\matrix-public-archive\server\render-hydrogen-to-string.js not supported.
Instead change the require of hydrogen.es.js in C:\Users\MLM\Documents\GitHub\element\matrix-public-archive\server\render-hydrogen-to-string.js to a dynamic import() which is available in all CommonJS modules.

@MadLittleMods
Copy link
Contributor Author

While trying to solve this, I think I stumbled upon what the #vite-bug was referring to -> vitejs/vite#6459. @bwindels looks like we both ran into the same issue 😁

MadLittleMods added a commit that referenced this issue Feb 26, 2022
Fix #686
Fix #682

Instead of deleting the whole `target/` directory, leave it alone so the symlink
driving the `npm link`/`yarn link` stays in tact.

Leave Vite builds in their build directories (`/lib-build`/`/asset-build`)
so you can `vite build --watch` to build on local changes and still have a
consisent place to reference in the `package.json` `exports`. Previously,
everything relied on `build.sh` which does a bunch of moving and renaming
and made it hard to rebuild on changes.

Add back support for CommonJS (adding the `package.json` `exports`).

The last piece is making sure the `?url` imports (`import workerPath from 'hydrogen-view-sdk/main.js?url';`)
work still. It looks like this may have just been solved via
vitejs/vite#6725 -> vitejs/vite#7073
(literally 2 days ago) and we just need to wait for the next Vite release 🎉
@MadLittleMods MadLittleMods self-assigned this Feb 26, 2022
@MadLittleMods
Copy link
Contributor Author

It looks like this may have just been fixed by vitejs/vite#6725 -> vitejs/vite#7073 (literally 2 days ago) and we just need to wait for the next Vite release 🎉

Re-introduced the CommonJS exports in #693 and will try it out the new Vite release

@MadLittleMods
Copy link
Contributor Author

Commented on the status of this in the PR #693 (comment)

I played around with that fix and it still doesn't fix the situation for *.html?url or non-ESM .js?url imports, see vitejs/vite#7097

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants