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

ESM support #83

Open
angelaraya opened this issue Mar 8, 2022 · 11 comments · May be fixed by #145
Open

ESM support #83

angelaraya opened this issue Mar 8, 2022 · 11 comments · May be fixed by #145

Comments

@angelaraya
Copy link

Issue type:

  • ➕ Feature request

Description:

With the move to ESM on the server side, there are packages that will stop supporting (or right out removed) CommonJS support. This means that any dependency that only exports as an ES module won't be usable with Components.js.


Some more context

I ran into this issue recently when trying to use sai-js in the Interop Authorization Agent implementation and would appreciate not having to change the codebase to use dynamic imports for no other reason than having to comply with CommonJS.

@github-actions
Copy link

github-actions bot commented Mar 8, 2022

Thanks for the suggestion!

@rubensworks
Copy link
Member

Components.js currently only supports CommonJS modules indeed.
The architecture is however ready for ESM support, so it's just a matter of implementing it.

This is not part of my current roadmap at the moment though (since my projects are all on CJS atm). But I'm happy to give guidance for anyone that wants to submit a PR. (Placing a bounty via the Comunica Association might also be a possibility)


An easier solution might however be to just expose both the CJS and ESM versions of the sai-js module in its package.json file, which could be done like this: https://github.com/RubenVerborgh/AsyncIterator/blob/main/package.json#L8-L14

@rubensworks
Copy link
Member

For reference, adding support for loading ESM modules should just be a matter of implementing a new IConstructionStrategy. Currently, we have a ConstructionStrategyCommonJs.ts, where we would require a ConstructionStrategyEsm in the future.

Most likely, only the implementation of createInstance will be different in ConstructionStrategyEsm compared to ConstructionStrategyCommonJs. So the implementation should be fairly straightforward.

However, if the desire is to also use Components-Generator.js on ESM projects, that may also require some tweaks in its ResolutionContext

@elf-pavlik
Copy link

We were trying to add Conditional exports to sai-js janeirodigital/sai-js#32 but that doesn't seem to solve the problem.

@woutermont based on the comments above, do you think you would be able to put together a PR with basic support for ESM?

@angelaraya will you need to use components-generator on sai-js or it is just a matter of IConstructionStrategy?

@rubensworks
Copy link
Member

We were trying to add Conditional exports to sai-js janeirodigital/sai-js#32 but that doesn't seem to solve the problem.

@elf-pavlik Does it produce the same error as before?
Have you tried adding an export entry for package.json, as is done here?: "./package.json": "./package.json",
(Some background on why that is needed: RubenVerborgh/AsyncIterator#34)

@woutermont
Copy link
Contributor

@elf-pavlik Digita has some packages which use conditional exports succesfully to work both with ComponentsJS and in the browser, so i.m.o. that should work for sai-js as well. Do you have an open PR for that somewhere?

I will try to look into ESM support next week. Did not think about the generator implications though, so it might be more complex than I expected.

@angelaraya
Copy link
Author

@woutermont @rubensworks the error with the conditional exports was fixed in sai-js. The only open item would be adding ESM support for componentsjs and the generator.

@woutermont
Copy link
Contributor

Okay, great! So we're temporarily good for sai-ja, as long as we don't need esm-only dependencies, right?

@elf-pavlik
Copy link

Has anyone had a chance to look further into adding ESM support?

@rubensworks
Copy link
Member

No, not that I'm aware of.

@elf-pavlik
Copy link

I was updating the testing setup in https://github.com/o-development/solid-notification-client
To start the latest CSS, everything went well until execution reached the point where CSS dynamically imports oidc-provider, which is an ESM package, where it began throwing errors, complaining about import statements.

I think supporting ESM could prevent needing to sandwich projects like CSS as CJS between otherwise standard ESM code.

@jeswr jeswr linked a pull request Sep 2, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants