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

Simplify build in 3.0.0 #1525

Open
thought2 opened this issue Jul 3, 2021 · 8 comments
Open

Simplify build in 3.0.0 #1525

thought2 opened this issue Jul 3, 2021 · 8 comments
Milestone

Comments

@thought2
Copy link
Contributor

thought2 commented Jul 3, 2021

Version 3.0.0 comes with an improved build process.
Currently it does some code patching of import statements, as the output file structure gets modified.
The reason for this is that every module needs to exists inside a directory, as it contains a "packaga.json", and source files for "lib", "es6" as well as "d.ts" typings.

E.g. see:
https://github.com/gcanti/fp-ts/blob/3.0.0/scripts/build2.ts#L49

The path of the "Alt" module is rewritten to "Alt/Alt".

I was wondering if the code rewriting can be omitted when paths are normalized like so:
src/foo/bar/baz.ts -> dist/foo/bar/baz/index.js

Looks like less intrusive, but maybe I oversee something.

@ryota-ka
Copy link
Contributor

ryota-ka commented Sep 8, 2021

Hi @gcanti, if you think the distinction between lib/ vs es6/ to be an implementation detail and want to hide it away, Node.js's subpath exports and conditional exports might help the situation.

Instead of placing package.json per subdirectory, recent versions of Node.js offers subpath exports, a concise and standard (important!) way to specify how the library should be imported.
(Webpack 5 also supports it.)
This will prevent a library consumer from importing fp-ts/lib/* and force to import from fp-ts/*.

In addition to that, thanks to conditional exports, lib/ and es6/ will automatically be selected.

If we adopt them, the package.json on the package root will look like this:

{
  "exports": {
    "import": "./es6/index.js",
    "require": "./lib/index.js",
    "./Alt": {
      "import": "./es6/Alt.js",
      "require": "./lib/Alt.js"
    },
    // ...
  }
}

Though the current "multiple package.json" style works well, there is a problem decreasing developers' experience: the language server inserts an import statement not from fp-ts/* but from fp-ts/lib/*.
I'm not sure the if language server already supports these features (I believe it does though), this change should make it easier to interact with fp-ts.
So why not moving to it on v3?

@samhh
Copy link
Contributor

samhh commented Sep 8, 2021

^ I was playing about with this for fp-ts-std. I haven't tested it much yet but it's worth seeing if we could take advantage of wildcards in conditional exports:

{
  "exports": {
    "./*": {
      "require": "./lib/*.js",
      "import": "./es6/*.js"
    }
  }
}

Edit: In use over there as of 0.12!

@raveclassic
Copy link
Collaborator

@ryota-ka Great suggestion! This is one of the things I would really like to see in fp-ts@3

@TylorS
Copy link

TylorS commented Sep 11, 2021

I also think this makes a lot of sense to do. Typescript should understand package.json exports in their next release.
microsoft/TypeScript#45418

@gcanti gcanti added this to the 3.0.0 milestone Sep 15, 2021
@gcanti
Copy link
Owner

gcanti commented Mar 23, 2022

@ryota-ka @samhh thanks for your suggestions, just released fp-ts@3.0.0-alpha.20 (npm i fp-ts@next), let me know if it's working as expected

@ryota-ka
Copy link
Contributor

@gcanti I tried it with typescript v4.6.3 and webpack v5.70.0.

  • "Go to definition" in VSCode works nice both with --module=CommonJS and --module=ESNext --moduleResolution=Node
  • Node.js also loads the correct module when using CommonJS
  • Webpack seems to have resolved the correct module corresponding to the module option in tsconfig.json

Aside, files under esm/ directory are not valid ESM files in fact (import statements need file extensions in ESM), so it's still impossible to import ESM files from Node.js.
I hope it'll be resolved with coming --module=Node12 option.

@samhh
Copy link
Contributor

samhh commented Mar 28, 2022

I've noticed in 3.0.0 branch there's different targets in the relevant tsconfigs. I'm not sure this makes sense: I may be using a modern bundler but need libraries to ship with older browser support, or I may be using an old bundler but not need browser support going back to ES5.

My approach was to unify each around the minimum of browser support and Node.js support however I may have overlooked something.

@imcotton
Copy link
Contributor

imcotton commented Apr 1, 2022

As mentioned by @ryota-ka, current esm/ is invalid to be consumed by ESM natively, I think easiest fix would be change the default entry '.' to CommonJS-only (before the extensions issue been fully addressed).

in:

fp-ts/scripts/build3.ts

Lines 39 to 44 in 799d80b

exports: {
'.': {
require: './cjs/index.js',
import: './esm/index.js'
},
'./*': {

change to:

exports: { 
  '.': './cjs/index.js',
  './*': { 

It's not ideal but at least gains interoperability from ESM to CommonJS.

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

7 participants