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

Drop CJS #1777

Open
gcanti opened this issue Sep 16, 2022 · 34 comments
Open

Drop CJS #1777

gcanti opened this issue Sep 16, 2022 · 34 comments
Milestone

Comments

@gcanti
Copy link
Owner

gcanti commented Sep 16, 2022

Linking the same issue in effect-ts.

As you may already know, there's an ongoing effort to find common ground with effect-ts and unify the ecosystem.

Effect will be ESM only from 31/12/2022.

I propose to switch to ESM only (fp-ts v3 and its alpha releases).

@gcanti gcanti added this to the 3.0.0 milestone Sep 16, 2022
@steida
Copy link
Contributor

steida commented Sep 17, 2022

I suggested that several months ago and @mikearnaldi told me it's absolutely a no-go 😅 It's great he changed his mind. I am all for ESM only because of my own experiences writing a lib in TypeScript with a few Web workers, several dependencies, etc. I finally managed to configure ESM by not using "bundlers" because it was impossible to do with them (you can't just inline web worker using ESM dependencies, the only bundlers capable of doing that is Webpack, and in Next.js, it works out of the box). Now I am challenging another problem with Node.js. I can't just use ESM only library in Node 16 ESM. So, we must ensure that fp-ts ESM will work in Node.js ESM. Once a lib is written in ESM, Node ESM is a must. You can't import ESM in CJS while you can import CJS in ESM. Slightly off-topic, I am contemplating rewriting my lib to Deno and provide a special build for npm and Node.js. Related discussion #1782

@kungfooman
Copy link

I got bundler-free ESM to work from /dist/, but only after basically fixing every import statement:

image

It's a pity that TypeScript can't just add *.js to make things works as they should. It should be part of the fp-ts build process.

Used regular expressions:

SEARCH: (\bfrom\s+["']\..*)(["'])
REPLACE: $1.js$2
FILES TO INCLUDE: dist/es6/*.js

@mikearnaldi
Copy link
Contributor

I suggested that several months ago and @mikearnaldi told me it's absolutely a no-go 😅 It's great he changed his mind. I am all for ESM only because of my own experiences writing a lib in TypeScript with a few Web workers, several dependencies, etc. I finally managed to configure ESM by not using "bundlers" because it was impossible to do with them (you can't just inline web worker using ESM dependencies, the only bundlers capable of doing that is Webpack, and in Next.js, it works out of the box). Now I am challenging another problem with Node.js. I can't just use ESM only library in Node 16 ESM. So, we must ensure that fp-ts ESM will work in Node.js ESM. Once a lib is written in ESM, Node ESM is a must. You can't import ESM in CJS while you can import CJS in ESM. Slightly off-topic, I am contemplating rewriting my lib to Deno and provide a special build for npm and Node.js. Related discussion #1782

Indeed it was a no-go, at the time if I recall correctly there wasn't even an LTS of node with ESM out, that said even today it will piss off a lot of people I am fairly sure of that BUT I think we have done enough supporting for so long both and we need a push

@mikearnaldi
Copy link
Contributor

I got bundler-free ESM to work from /dist/, but only after basically fixing every import statement:

image

It's a pity that TypeScript can't just add *.js to make things works as they should. It should be part of the fp-ts build process.

Used regular expressions:

SEARCH: (\bfrom\s+["']\..*)(["'])
REPLACE: $1.js$2
FILES TO INCLUDE: dist/es6/*.js

I don't think the build process should touch imports, we have really 2 choices here:

  1. add .js to every import/export
  2. use fully qualified imports like fp-ts/Array

There was a long discussion in TS about adding .js automatically and it was decided not to do so as TS is no longer accepting any term level change to JS

@kungfooman
Copy link

There was a long discussion in TS about adding .js automatically and it was decided not to do so as TS is no longer accepting any term level change to JS

Yea, and the joke is on them, what do they expect? Import a TS file with JS extension? Their stance is unproductive and blind to reality...

Anyway, we can't change TS but what I suggested is basically your point (1)... just a little script fixing the imports in dist/es6 once they are generated, so ESM actually works as first-class citizen in any modern browser without any bundler.

@steida
Copy link
Contributor

steida commented Sep 18, 2022

@kungfooman haha, I just finished reading all that comments. I think what they want is https://github.com/tc39/proposal-type-annotations so there will be js everywhere in the end.

@mikearnaldi Did you try mts? The advantage should be it enforces suffixes, and no further procession is required, I suppose.

@steida
Copy link
Contributor

steida commented Sep 18, 2022

So I tried mts, and as a result, I got weird TypeScript errors in Node using it, jeez...

Btw, TypeScript allows .js extension, so rewriting is not required, only checking.

https://github.com/koskimas/kysely/blob/master/scripts/check-esm-imports.js

Also, I tried this eslint plugin https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/extensions.md, but it does not work as expected for some reason.

@mikearnaldi
Copy link
Contributor

@kungfooman haha, I just finished reading all that comments. I think what they want is https://github.com/tc39/proposal-type-annotations so there will be js everywhere in the end.

@mikearnaldi Did you try mts? The advantage should be it enforces suffixes, and no further procession is required, I suppose.

I did but the d.ts emitted as d.mts are very bothering to me, also it compiles to mjs not js.

Overall the best set of tradeoffs I found is by using fully qualified imports.

@steida
Copy link
Contributor

steida commented Sep 18, 2022

@mikearnaldi
Copy link
Contributor

There was a long discussion in TS about adding .js automatically and it was decided not to do so as TS is no longer accepting any term level change to JS

Yea, and the joke is on them, what do they expect? Import a TS file with JS extension? Their stance is unproductive and blind to reality...

Anyway, we can't change TS but what I suggested is basically your point (1)... just a little script fixing the imports in dist/es6 once they are generated, so ESM actually works as first-class citizen in any modern browser without any bundler.

Yes, the 'little script' may break other things though, it's not a really good practice to run regex replace on code, if any a tsc plugin or something of the sort of an ast plugin (if using swc/esbuild) would be better. Also no the joke is not on them, it's what has been decided and there was a long enough explanation about why that is needed, to me the bad decision is upfront in js not really in ts as again when writing ts you write term valid js (design goal)

@mikearnaldi
Copy link
Contributor

@mikearnaldi #1777 (comment)

What do you mean by 'ts allows js'?

@steida
Copy link
Contributor

steida commented Sep 18, 2022

You can use .js extension in .ts file. It looks weird, but tsc compiles it properly. I suppose it's future compatible when https://github.com/tc39/proposal-type-annotations became a reality. Then we will rename all .ts files to .js, and everything will be nice and all again 😂

@mikearnaldi You did not get it; that script does not rewrite anything; it's only a check.

@steida
Copy link
Contributor

steida commented Sep 18, 2022

@mikearnaldi microsoft/TypeScript#16577 (comment) ".js file extensions are now allowed"

@mikearnaldi
Copy link
Contributor

@steida yes I am aware, it was part of the esm integration, as of my mention of a script I was not referring to the script you posted but to the proposal of 'It should be part of the fp-ts build process.' by @kungfooman which I don't agree with, I think as mentioned above that we have 2 choices either stay with relative imports but adding the '.js' extension or use fully qualified imports that use the package.json lookup logic

@steida
Copy link
Contributor

steida commented Sep 18, 2022

Meanwhile, I tested two eslint plugins for checking file extensions. Guess what, no one works.

This one check also packages, can't be disabled.
https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/file-extension-in-import.md

This does not work either.
https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/extensions.md

@steida
Copy link
Contributor

steida commented Sep 18, 2022

Funny thing fp-ts@2.12.3 with Node 16 ESM:

I need pipe. I can't use import { pipe } from "fp-ts/function" of course.
But I can't use import { pipe } from "fp-ts/es6/function.js" either.

server:start: file:///Users/danielsteigerwald/dev/evolu/apps/server/dist/index.js:1
server:start: import { pipe } from "fp-ts/es6/function.js";
server:start:          ^^^^
server:start: SyntaxError: Named export 'pipe' not found. The requested module 'fp-ts/es6/function.js' is a CommonJS module, which may not support all module.exports as named exports.

For some reason, importing from lib works import { pipe } from "fp-ts/lib/function.js"

And it's transitive, so I suppose many fp-ts libraries will not work with Node 16 ESM.

And because my library is ESM only, I can't import it as CommonJS.

(node:39713) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.

I hope fp-ts 3 will be released soon 🙂

@mikearnaldi
Copy link
Contributor

mikearnaldi commented Sep 19, 2022

Funny thing fp-ts@2.12.3 with Node 16 ESM:

I need pipe. I can't use import { pipe } from "fp-ts/function" of course. But I can't use import { pipe } from "fp-ts/es6/function.js" either.

server:start: file:///Users/danielsteigerwald/dev/evolu/apps/server/dist/index.js:1
server:start: import { pipe } from "fp-ts/es6/function.js";
server:start:          ^^^^
server:start: SyntaxError: Named export 'pipe' not found. The requested module 'fp-ts/es6/function.js' is a CommonJS module, which may not support all module.exports as named exports.

For some reason, importing from lib works import { pipe } from "fp-ts/lib/function.js"

And it's transitive, so I suppose many fp-ts libraries will not work with Node 16 ESM.

And because my library is ESM only, I can't import it as CommonJS.

(node:39713) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.

I hope fp-ts 3 will be released soon 🙂

I don't think v2 was meant to be compatible with ESM as it doesn't respect the following: a .js file is identified as CJS if the nearest package.json doesn't have type: "module" and a .js file is identified as ESM if the nearest package.json is a module.

In effect to have both working I use:

  1. fully qualified imports so no .js extension added but @effect/core/...
  2. a pipeline that produces cjs in the main package root (compat with pre-exports clients like old node)
  3. a pipeline that produces esm that also renames files to .mjs and because I don't use relative imports they need no change

to have the same working with relative imports it would be needed to change all the relative imports to point to the correct mjs/js extension

At the moment v2 is only ok for bundlers

@steida
Copy link
Contributor

steida commented Sep 19, 2022

@mikearnaldi Can you please repeat the problem with writing .mts from the start? It enforces the extension and should work well. I had some issues with types, but the same issues I had with .ts so the problem was elsewhere I guess.

@mikearnaldi
Copy link
Contributor

@mikearnaldi Can you please repeat the problem with writing .mts from the start? It enforces the extension and should work well. I had some issues with types, but the same issues I had with .ts so the problem was elsewhere I guess.

I don't quite like to have mts types they are incompatible with older ts and yes it emits mjs which are fine but if I want mjs in the extension it is usually for cases where both mjs and cjs needs to be emitted and that means always an additional pipeline to fix up the imports, instead in a pure module (type module) plain js extensions are fine. So to summarise I don't dislike mts / cts but I just don't have any situation where they actually help, they would help a lot in a hybrid package where you keep state in cjs and have mjs wrappers to support importing both in the same project (mjs can import cjs and you may want to share state) but I don't really suggest that as a good idea I think we should migrate to a world where we only have one (esm)

@steida
Copy link
Contributor

steida commented Sep 21, 2022

As for testing... I just spend a few hours trying to force Jest to work with ESM. No success. And I'm not alone.

Then I switched to Vitest (like the others). It worked immediately without any configuration.

@kungfooman
Copy link

So how many hours did you waste now by doing it "properly" by testing all kinds of plugins with extra bells and whistles?

I got ESM to work with ONE regex 😂

Good job I guess, professionals...

@mikearnaldi
Copy link
Contributor

So how many hours did you waste now by doing it "properly" by testing all kinds of plugins with extra bells and whistles?

I got ESM to work with ONE regex 😂

Good job I guess, professionals...

I have ESM working since ages with absolutely no workaround. Also we're all here to help each other, that kind of comment you can keep for yourself.

@viell-dev
Copy link

I will recommend still building to .js and .d.ts files and using type: module.

There seems to be better support for extensionless imports with .js and .d.ts files. If you have sections under exports that use wildcards and also point to files with the new explicit extensions. Then they seem to require extensions when importing.

I don't know if this is just a moduleResolution: NodeNext thing or what. But I've noticed this when importing some packages that use .mjs and .d.mts.

@azat-io
Copy link

azat-io commented Nov 2, 2022

I'm using "moduleResolution": "nodenext" with "type": "module". At the moment I am getting the following error:

import { pipe } from "fp-ts/function";
         ^^^^
SyntaxError: Named export 'pipe' not found. The requested module 'fp-ts/function' 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/function';
const { pipe } = pkg;

My version of fp-ts is 3.0.0-alpha.26.

@mikearnaldi
Copy link
Contributor

mikearnaldi commented Nov 2, 2022 via email

@azat-io
Copy link

azat-io commented Nov 3, 2022

@mikearnaldi It works! Thanks!

Off topic: Why are all the useful things in @fp-ts/data? What is @fp-ts/core for?

@mikearnaldi
Copy link
Contributor

mikearnaldi commented Nov 3, 2022

@mikearnaldi It works! Thanks!

Off topic: Why are all the useful things in @fp-ts/data? What is @fp-ts/core for?

You're welcome!

So the idea is to separate a bit the implementations to keep it more maintainable and focused, in /core we have only abstractions that general but that at the same time are rarely useful from the user perspective, a normal user will interact mostly with specific packages, the plan is:

  • @fp-ts/data => all the common data structures
  • @fp-ts/codec => similar to io-ts focused on domain modelling
  • @fp-ts/optic => similar to monocle
  • @effect/io => effect execution
  • @effect/stream => streaming focused
  • etc

You can check the repositories in the orgs, in the one for /core there is some kind of initial explainer of what we have in mind:

@mikearnaldi
Copy link
Contributor

mikearnaldi commented Nov 3, 2022

I will recommend still building to .js and .d.ts files and using type: module.

There seems to be better support for extensionless imports with .js and .d.ts files. If you have sections under exports that use wildcards and also point to files with the new explicit extensions. Then they seem to require extensions when importing.

I don't know if this is just a moduleResolution: NodeNext thing or what. But I've noticed this when importing some packages that use .mjs and .d.mts.

Yeah the mts extension is kind of problematic, with @fp-ts/core we are currently publishing a dual setup with plain .js in the root indexed in a way that is compatible with older systems, then via the exports package we expose the .mjs modules for esm usage which maximise compatibility with every possible setup.

Unfortunately using type: module is still not widely compatible, the whole thing is very messy

@yuriy-yarosh
Copy link

I'd say that "type": "module" is still very intrusive - you're forced convert whole codebase to ESM.
Even .babelrc.mjs configs with "Experimental babel async workersfor@babel/register` etc.

I'm still dual-building projects with a mix of babel-preset-typescript due to hard dependencies on babel-macro in certain conditions... and code coverage issues with type-coverage and C8.

Having a "type": "commonjs" project with occasional isolated "exports": { ... }, for a "type": "module" dependency, is fine. So, if fp-ts would really want to switch to "type": "module", I'd stick to the official Isolated modules recommendation first, for the nodenext resolver.

@kungfooman
Copy link

kungfooman commented May 29, 2023

I'd say that "type": "module" is still very intrusive - you're forced convert whole codebase to ESM.

You don't even need "type": "module", just use file extension *.mjs. But if you set it, you can simply drop out via file extension *.cjs. I don't know what's intrusive about that, just use proper file extensions.

IMO main issue is still TypeScript, just rewrite everything in JS with JSDoc and get rid of the ideological TS BS... this also makes the JS hidden under the pile of interspersed types readable again. 😂

@yuriy-yarosh
Copy link

@kungfooman transpiling mts/mjs in babel with a mixed esm/cjs setup is very quirky, and often rely on async babel usage, especially for @babel/register. TSC / esbuild / swc аre completely different beasts, in that regard, altogether.

@jderochervlk
Copy link

Hi! I know active development has moved to effect, but I'd be willing to tackle a setup for the repo to have dual CJS/ESM support. It would also solve this issue #1910

I'll start working on a PR and hopefully have it ready by the end of the week.

@gcanti
Copy link
Owner Author

gcanti commented Apr 3, 2024

We've already tried it in the past, and it didn't go well. Shortly after the release, I received several open issues regarding setup problems, so I decided to revert the change and postpone the operation to the next major release.

@jderochervlk
Copy link

Will there be a major release for fp-ts? Or will these changes only apply to Effect?

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

No branches or pull requests

8 participants