samhh / fp-ts-std

The missing pseudo-standard library for fp-ts.
https://samhh.github.io/fp-ts-std/
MIT License
207 stars 27 forks source link

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

Closed andreavaccari closed 1 year ago

andreavaccari commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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! :smile:

andreavaccari commented 2 years ago

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
  2. Replace src/routes/index.svelte with:

    
    <script lang="ts">
    import { invert } from "fp-ts-std/Boolean";
    </script>

{invert(false)}


3. Run `npm run dev`

This will run without issues.


4. 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](https://github.com/sveltejs/kit/issues/3246) (see also [library issue](https://github.com/heroiclabs/nakama-js/issues/107) and [library PR](https://github.com/heroiclabs/nakama-js/pull/108)) 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 commented 2 years ago

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 commented 2 years ago

Thank you so much for your help, @bluwy!

samhh commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

.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 commented 2 years ago

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 commented 1 year ago

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

andreavaccari commented 1 year ago

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 commented 1 year ago

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

samhh commented 1 year ago

Closing, fixed ESM can be tracked at #193.