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

Cannot import via ES Module #37

Open
Jack-Works opened this issue Sep 17, 2021 · 11 comments · May be fixed by #49
Open

Cannot import via ES Module #37

Jack-Works opened this issue Sep 17, 2021 · 11 comments · May be fixed by #49

Comments

@Jack-Works
Copy link
Contributor

node -v
v16.8.0

cat .\x.mjs

import {Err} from 'ts-results'

node .\x.mjs

import {Err} from 'ts-results'
        ^^^
SyntaxError: Named export 'Err' not found. The requested module 'ts-results' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'ts-results';
const {Err} = pkg;
@jstasiak
Copy link
Contributor

Same here, Node.js 17.5.0, pure ESM package, node --experimental-vm-modules enabled and ts-results 3.3.0. I'd be happy to produce a patch for this but right now I have no idea how to make this package ESM-compatible while retaining CommonJS support.

jstasiak added a commit to lune-climate/ts-results-es that referenced this issue Mar 9, 2022
We have ESM package that needs to import ts-results and it's not easy at
the moment because of[1]

    import {Err} from 'ts-results'
            ^^^
    SyntaxError: Named export 'Err' not found. The requested module 'ts-results' is a CommonJS module, which may not support all module.exports as named exports.
    CommonJS modules can always be imported via the default export, for example using:

    import pkg from 'ts-results';
    const {Err} = pkg;

Since time is kind of of the essence and it's easier to make this
package ESM-only instead of trying to figure out how to handle both
CommonJS and ESM client code let's publish an ESM-only version for now.

[1] vultix#37
@jstasiak
Copy link
Contributor

jstasiak commented Mar 9, 2022

Hey all, since I really needed this addressed quickly I published an ESM-only version of this package to NPM under the name of ts-results-es. This is the change that makes the package importable by ESM packages: lune-climate@0be48c5

I don't intend the package to be a long running fork of ts-results and I'm happy to drop it the moment ts-results works nicely with ESM.

@Jack-Works
Copy link
Contributor Author

@jstasiak sorry but your version is still broken. change package.json is not enough, you need also to add all file extensions.

@jstasiak
Copy link
Contributor

Hey @Jack-Works I forgot to mention, I'm running node with --experimental-specifier-resolution=node enabled.

You're right the extensions are needed in general, this is good enough for my use case right now.

@jstasiak
Copy link
Contributor

@Jack-Works this should work without --experimental-specifier-resolution=node in both ESM and CJS modes: lune-climate#5

I pushed it to NPM as [email protected] and it'd be great if I got a confirmation it works for you. If it does I'll create a PR in this repository.

@vultix
Copy link
Owner

vultix commented Mar 11, 2022

Hey all, I apologize for the long delay getting a fix out here. I’ve set aside time this weekend to get a release out!

jstasiak referenced this issue in lune-climate/ts-results-es Mar 14, 2022
This change follows the solution described in [1] to make the package
play nice with both CommonJS and ESM clients.

This includes:

* Changing imports to use the .js extension explicitly
* Modifying the dist/package.json file to refer to different directories
  depending on the context
* Providing two package.json files that override the package type
  depending on the context

Fixes: GH-37

[1] https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html
@jstasiak jstasiak linked a pull request Mar 14, 2022 that will close this issue
@jstasiak
Copy link
Contributor

#49 should resolve this

jstasiak added a commit to lune-climate/ts-results-es that referenced this issue Mar 14, 2022
In [1] I made the package ESM-only to make it possible to import
it easily from Lune's ESM code. This is not great so I found a way
to restore the CommonJS compatibility while still resolving the
ESM-related issues[2] found in the original ts-results package.
The solution I found can be found in [3].

[1] 0be48c5 ("Make the package ESM-only")
[2] vultix#37
[3] https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html
@jacoobes
Copy link

jacoobes commented Aug 5, 2022

Great project btw, i love ts-results. Any update on esm / cjs compatibility?, been trying to interop esm and cjs targets with my typescript project, but its been tough due to imports / exports of ts-results.

@jstasiak
Copy link
Contributor

jstasiak commented Aug 5, 2022

For what it's worth you can use our ESM-compatible fork for the time being (https://www.npmjs.com/package/ts-results-es). I'd love the fork to become unnecessary though.

@jacoobes
Copy link

jacoobes commented Aug 5, 2022

For what it's worth you can use our ESM-compatible fork for the time being (https://www.npmjs.com/package/ts-results-es). I'd love the fork to become unnecessary though.

Oh, didn't see this. Much love, thank you so much! hoping this will be merged sooner or later

@tv42
Copy link

tv42 commented Sep 27, 2022

Today, I had to re-learn about this workaround just because it had been almost three months from the last round.. Can this get merged, pretty please?

jstasiak pushed a commit to lune-climate/ts-results-es that referenced this issue Jan 31, 2024
My main motivation for using ts-results-es over ts-results is compatibility
with esbuild: after running into the problem noted in this issue[1] and then
reading that discussion, I decided to try to use ts-results-es.

However, ts-results-es did not work out of the box in my bundling setup.
I need to produce a commonjs module (for compatibility with legacy
dependencies) from typescript/esm source, but
esbuild --bundle --platform=node --conditions=require was producing
a bundle that imported from ts-results-es/dist/esm instead of
ts-results-es/dist/cjs.

After some digging I determined that the issue is that esbuild checks
package.json.exports in the order that the key-value pairs appear:
source code[2], confirmation from author[3].

Since ts-results-es/package.json.exports lists import before require,
and both the import and require conditions are set since I am importing
from ts-results-es with an import but also set --conditions=require 
via the command line, import is chosen over require. This PR swaps
the order so that require is listed before import, so that esbuild
chooses the require condition for my bundle.

Theoretically, this could break the setup of someone else who is
importing ts-results-es using the legacy require syntax but is
producing an esm bundle, but such a setup seems highly unlikely
to me. My first thought is that it is much more likely that someone
(like me) would want to use modern syntax (import) and produce
a commonjs bundle for use with legacy packages or systems.
I also inquired on that thread[4] whether using legacy require syntax
and bundling for esm is a legitimate or common use case.


[1] vultix#37
[2] https://github.com/evanw/esbuild/blob/main/internal/resolver/package_json.go#L1148
[3] evanw/esbuild#3166 (comment)
[4] evanw/esbuild#3166 (comment)
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 a pull request may close this issue.

5 participants