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

eslint-plugin-import/no-unresolved error caused by conditional exports #113

Closed
andreavaccari opened this issue Mar 5, 2022 · 14 comments
Closed

Comments

@andreavaccari
Copy link

Hi @samhh, thank you for your work. I'm aware of the pinned issue that discusses the interaction between jest and fp-ts-std's conditional exports. I'm having a similar issue with the import plugin for eslint. The import/no-unresolved rule complains that it is unable to resolve path to module 'fp-ts-std/Array.

This issue discusses ways to mitigate the problem. Would you consider applying the same recommendations here?

Thank you!

@andreavaccari
Copy link
Author

Actually, this is not just an issue with the linter, our entire build fails on the following error:

(node:82794) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
/Users/.../node_modules/fp-ts-std/dist/esm/IO.js:1
import * as IO from "fp-ts/IO";
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at compileFunction (<anonymous>)
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1032:15)
    at Module._compile (node:internal/modules/cjs/loader:1067:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:168:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
    at async Promise.all (index 0)
SyntaxError: Cannot use import statement outside a module

@samhh
Copy link
Owner

samhh commented Mar 7, 2022

Regarding the plugin, I'm resistant to patch this library for legacy resolvers.

It was a challenge getting things working as they are now, and if memory serves backwards compatibility here complicates the build story to the extent that entire packages have been built to attempt to abstract it away.

Bundlers and Node each now support conditional exports. Jest looks set to do so soon and as you've noted is easily patched for the timebeing. Whilst the plugin author's justification for the delay in support is reasonable, I'd nonetheless consider this a bug on their end.

In the meantime, this may suffice as a workaround:

'import/no-unresolved': [2, { ignore: ['^fp-ts-std'] }],

Regarding your build/runtime, where's that output coming from and with what version(s)? I know the library does/can work with Webpack 5 and Node 16, though of course bundle configuration is always a joy! 😄

@andreavaccari
Copy link
Author

Hi @samhh , thank you for the quick follow-up. We use fp-ts and fp-ts-std with SvelteKit and Vite.

Here is how you can easily reproduce the issue.

  1. Run the following
npm init --yes @svelte-add/kit@latest -- --with typescript sveltekit-fp-ts-std
cd sveltekit-fp-ts-std
npm i fp-ts fp-ts-std
  1. Replace src/routes/index.svelte with:
<script lang="ts">
  import { invert } from "fp-ts-std/Boolean";
</script>

{invert(false)}
  1. Run npm run dev
This will run without issues.
  1. Run npm run build
import { invert } from "fp-ts-std/Boolean";
         ^^^^^^
SyntaxError: Named export 'invert' not found. The requested module 'fp-ts-std/Boolean' 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 'fp-ts-std/Boolean';
const { invert } = pkg;

Possible solution

This SvelteKit issue (see also library issue and library PR) seems equivalent and was solved by:

  1. Changing ESM modules' extensions from .js to .mjs.
  2. Exporting "./package.json": "./package.json"

I tried to apply this changes in my local installation but I wasn't able to get things to work.

I know how difficult it is to make code interoperable between ESM, CJS, and the various bundlers and I understand why you are reluctant to complicate things further. Hopefully this gave you some ideas on how this problem could be solved. Thank you!

@bluwy
Copy link

bluwy commented Mar 8, 2022

Hi, @andreavaccari summoned me. Making packages work with bundlers and node can certainly be a challenge, but I think there's certain improvements can be made as @andreavaccari suggested. Even with conditional exports, the file extension matters too, here's how Nodejs determines what file is considered ESM/CJS. So ideally you can have two ways to fix this in your library:

  1. Change all dist ESM files to end with .mjs (preferred)
  2. or add package.json with {"type":"module"} in dist/esm

The "./package.json": "./package.json" export isn't needed now. With these changes it should work in Nodejs properly, which Vite emulates its algorithm, hence the error appears in Vite.

@andreavaccari
Copy link
Author

Thank you so much for your help, @bluwy!

@samhh
Copy link
Owner

samhh commented May 7, 2022

Thanks for the info and sorry for the delay.

I'd be open to patching in ESM support (file extensions and import suffixes) following the call to tsc when building, however I can't see any .mjs files or a "type": "module" key anywhere in fp-ts' output. Can an ESM library/project import from another that's non-ESM, or is that a blocker?

@andreavaccari
Copy link
Author

Hi Sam, I'll be unable to help for the next 4 weeks but I'd be happy to working with you on this afterwards.

@andreavaccari
Copy link
Author

andreavaccari commented Jul 5, 2022

@bluwy could you explain why changing EMS extensions to .mjsis preferred to adding a second package.json with {"type:""module"}? My understanding is that it's not possible to use tsc 1) to emit files with .mjs extensions and 2) to transform extensionful relative imports (e.g. import ... "./utils.js" to import ... "./utils.mjs", which is required by Node's module resolution algorithm). For #126 I ended up using tsup which takes care of both requirements, but, for my own education, I'd like to understand why you recommend one option over the other. Thank you!

@bluwy
Copy link

bluwy commented Jul 5, 2022

.mjs is preferred mostly because it's faster for internal implementations to detect whether a file is ESM or not. Using {"type":"module"}, they'd otherwise have to walk up the filesystem to find a package.json to detect it. Explicit extensions is also easier for developers to recognize the file type.

Good point about tsc and the explicit extensions needed in imports (especially for nodenext target). It should be fine to use {"type":"module"} too if you'd want to stick with tsc, but tsup is also a great option with nice defaults builtin.

Also I've made https://publint.bjornlu.com/fp-ts-std@0.14.2 recently which might help verify this 🙂

@andreavaccari
Copy link
Author

Thank you for the quick reply! I've learned a lot from your comments and answers here and in other repos. I used publint to confirm #126 is properly configured — it's a great tool! It showed that fp-ts and io-ts are also misconfigured. I plan to submit PRs for those packages after seeing how this change works in fp-ts-std.

@samhh
Copy link
Owner

samhh commented Jun 30, 2023

If this issue is still relevant, might I ask if 0.18.0-beta.1 resolves it for you?

@andreavaccari
Copy link
Author

We have been using patch-package to modify fp-ts and fp-ts-std and make them behave nicely in our monorepo.

Otherwise, the issue is still relevant.

bluwy's publint shows that you have resolved most issues in 0.18 compared to 0.17:

@bluwy
Copy link

bluwy commented Jul 1, 2023

That one warning is a bug on my end. Should be fixed now (with no warnings)

@samhh
Copy link
Owner

samhh commented Jul 4, 2023

Closing, fixed ESM can be tracked at #193.

@samhh samhh closed this as completed Jul 4, 2023
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.

3 participants