search

rescript-lang / rescript-compiler

The compiler for ReScript.
https://rescript-lang.org
Other
6.59k stars 440 forks source link

@rescript/core package doesn't have the javascript file #6754

Open TristanCacqueray opened 1 month ago

TristanCacqueray commented 1 month ago

Hello folks,

I'm trying to update one of my package (repo) from bs-platform 8.2 to rescript 11, and I get the following error when importing it in a regular javascript project:

Module not found: Can't resolve '@rescript/core/src/Core__List.res.js'

According to https://rescript-lang.org/docs/manual/latest/build-external-stdlib we need to use @rescript/std, and even though that removes the requirements from rescript, some modules from @rescript/core are not readily available as JavaScript. Is there something I am missing?

Thanks in advance, -Tristan

fhammerschmidt commented 1 month ago

rescript-core only ships with mjs files.

You could try to change your rescript.json to 

{
  ...
  "package-specs": [
    {
-      "module": "commonjs",
+      "module": "esmodule",
      "in-source": true
    }
  ],
-      "module": ".res.js",
+      "module": ".res.mjs",
  ...
}
TristanCacqueray commented 1 month ago

@fhammerschmidt Thanks, using .res.mjs still failed with:

Module not found: Can't resolve '@rescript/core/src/Core__List.res.mjs'

Using .mjs directly worked, but now the build is failing with:

Module not found: Can't resolve 'react/jsx-runtime'

Re-running yarn install in the downstream project fixed that error, but that results in:

./node_modules/@softwarefactory-project/re-ansi/src/Ansi.mjs
Can't import the named export 'jsx' from non EcmaScript module (only default export is available)

Finally, it seems like rescript is still a required dependencies to fix that other error:

./node_modules/@rescript/core/src/Core__Option.mjs
Cannot find module: 'rescript/lib/es6/caml_option.js'. Make sure this package is installed.
TristanCacqueray commented 1 month ago

Also switching to "jsx": {"version": 4, "mode": "classic"} results in:

./node_modules/@softwarefactory-project/re-ansi/src/Ansi.mjs
Can't import the named export 'createElement' from non EcmaScript module (only default export is available)

Note that this happens with a regular cra react-app-rewired project, not nextjs.

TristanCacqueray commented 1 month ago

Ok sorry about the noise, but if I understand correctly, create-react-app does not support mjs files out of the box according to https://github.com/facebook/create-react-app/issues/10356 . It would be better if we don't have to update downstream build script just for that.

Would it be possible to publish the javascript version of the std libs so that we can upgrade to rescript seamlessly?

fhammerschmidt commented 1 month ago

Ah yes, just .mjs is what I meant.

Sorry, I guess you need to still ship the whole runtime for now since ReScriptCore did not really take into account that feature yet.

It will be seamlessly when Core is part of the compiler.

Also, just a recommendation: Consider switching to vite, it's so much better and easier to maintain than create-react-app and esmodule should be the default over commonjs already IMO.

And when you use esmodule your end-users should be able to tree-shake all unused code away.

TristanCacqueray commented 1 month ago

Well I'm not familiar with react/vite/next, and my issue is that using mjs is a breaking change for CRA users.

It looks like .mjs is the way to go nowadays, and for what it's worth, here is the necessary change for react-app-rewired users:

// In config-overrides.js add:
function supportMJS(config) {                                                                                                                                                                                                                                                                                                                                                              
   config.module.rules.push({                                                                                                                                                                                                                                                                                                                                                               
     test: /\.mjs$/,                                                                                                                                                                                                                                                                                                                                                                        
     include: /node_modules/,                                                                                                                                                                                                                                                                                                                                                               
     type: "javascript/auto"                                                                                                                                                                                                                                                                                                                                                                
   });                                                                                                                                                                                                                                                                                                                                                                                      
   return config;                                                                                                                                                                                                                                                                                                                                                                           
}  

const { override } = require("customize-cra");

module.exports = override(supportMJS, rewiredEsbuild());

That will do, thanks for the explanation @fhammerschmidt .

fhammerschmidt commented 1 month ago

Don't you still have this issue?

Finally, it seems like rescript is still a required dependencies to fix that other error:

./node_modules/@rescript/core/src/Core__Option.mjs
Cannot find module: 'rescript/lib/es6/caml_option.js'. Make sure this package is installed.
TristanCacqueray commented 1 month ago

Don't you still have this issue?

Finally, it seems like rescript is still a required dependencies to fix that other error:

./node_modules/@rescript/core/src/Core__Option.mjs
Cannot find module: 'rescript/lib/es6/caml_option.js'. Make sure this package is installed.

No, this is gone after adding rescript to the dependencies instead of devDependencies.

fhammerschmidt commented 1 month ago

On second thought, you should really really use a bundler for publishing the library as well. Check out nanobundle for instance: https://forum.rescript-lang.org/t/bundle-rescript-libraries-with-dual-package-exports/4058

And I suggest we mention that on the rescript-std page on rescript-lang.org but I will do that myself.

TristanCacqueray commented 1 month ago

Sounds great to me, thanks for your help!

TristanCacqueray commented 1 month ago

@fhammerschmidt where you able to update the rescript-lang doc on how to do that? Is it possible to not bundle react and let the downstream project link with the version it uses?

fhammerschmidt commented 1 month ago

I fiddled around with nanobundle on your repo. I think for JS consumers the most viable path is to bundle all ReScript dependencies and only require react to be installed. But that is pretty specific to your use-case. I don't want to create a guide without really having some practical experiences from the community.

Here's roughly what to do:

  1. npm install nanobundle --save-dev
  2. add a minimal tsconfig.json to your root (no TypeScript necessary, but nanobundle will need it too): 
    {
"compilerOptions": {
"target": "ES2022",
"module": "ES2022",
"strict": true,
"jsx": "preserve",
"rootDir": "src",
"outDir": "dist"
},
"include": ["src"]
}
  3. In package json set the "type" field to "module" and add a script: 
    "bundle": "npm run clean && npm run build && nanobundle build --clean --no-sourcemap --no-legal-comments --external=react"
  4. Also in package.json, move all ReScript dependencies to devDependencies, remove @rescript/std, we won't need it.
  5. In rescript.json, remove the "external-stdlib" entry
  6. Also to keep your bundled code as minimal as possible, use Rescript Core's methods, like String.length over Js.String.length or List.reverse->List.flat over Belt.List.reverse->Belt.List.flatten.
  7. Do npm run bundle
  8. You end up with a fairly small bundle since the bulk of the app would be React. Check out the generated file for a import * as JsxRuntime from "react/jsx-runtime"; which should be the only import in your case.
  9. Consider adding react as a peer dependency.

Edit: this would at least support the ESM users, I think. You can also create bundles for both esm and cjs if you need to. See "dual-package exports" section: https://github.com/cometkim/espub/tree/main/packages/nanobundle

TristanCacqueray commented 1 month ago

@fhammerschmidt That's amazing, thank you so much for helping me out here! The procedure works great and the bundle command makes a standalone 25kb ReAnsi.res.js file.

However, updating my downstream app still failed because it couldn't resolve react/jsx-runtime, it is still on react-16.13.1. Though, updating to react-16.14.0 (as explained in https://legacy.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html) seems to work.

Thanks again for your help, it's much appreciated!

cometkim commented 1 month ago

add a minimal tsconfig.json to your root (no TypeScript necessary, but nanobundle will need it too):

Actually you don't need tsconfig.json. nanobundle only use it to specify --root-dir and --out-dir (default: lib).

Also all dependencies and peerDependencies will be automatically specified as external, and in case you will need the react as peerDependencies.

It's glad to hear that nanobundle has helped the problem, but this is not an ideal for ReScript. I'm focusing on improving the library authoring experience on both espub and ReScript. ReScript should be enough to build a JavaScript (and TypeScript) libraries without relying on external tools.

See #6209 and #6210

TristanCacqueray commented 1 month ago

While the create-react-app build worked when using "type": "module" with ".res.js" filename, the jest test suite is now failing with this error:

FAIL src/containers/build/BuildOutput.test.jsx
  ● Test suite failed to run

    node_modules/@softwarefactory-project/re-ansi/src/Ansi.res.js:210
    import * as JsxRuntime from "react/jsx-runtime";
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      at new Script (node:vm:116:7)
      at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:537:17)
      at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:579:25)
      at Object.<anonymous> (src/containers/build/BuildOutput.jsx:18:1)

According to https://github.com/facebook/create-react-app/issues/9938 , adding --transformIgnorePatterns 'node_modules/(?!@softwarefactory-project)/' fixes that test, but then another unrelated test fails like this:

FAIL src/App.test.jsx
  ● Test suite failed to run

    TypeError: Cannot read properties of undefined (reading 'document')

      16 | import PropTypes from 'prop-types'
      17 | import { connect } from 'react-redux'
    > 18 | import * as d3 from 'd3'
         | ^
      19 | import { useHistory } from 'react-router-dom'
      20 |
      21 | import { makeJobGraphKey, fetchJobGraphIfNeeded } from '../../actions/jobgraph'

      at node_modules/d3/d3.js:8:26
      at Object.<anonymous> (node_modules/d3/d3.js:1:2)
      at Object.<anonymous> (src/containers/jobgraph/JobGraphDisplay.jsx:18:1)
      at Object.<anonymous> (src/containers/jobgraph/JobGraph.jsx:21:1)
      at Object.<anonymous> (src/pages/Project.jsx:26:1)
      at Object.<anonymous> (src/routes.js:19:1)
      at Object.<anonymous> (src/App.jsx:67:1)
      at Object.<anonymous> (src/App.test.jsx:24:1)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

When using ".mjs" extension, the tests pass (though with this warning Warning: <Ansi.mjs /> is using incorrect casing.) but the build fails with:

$ yarn build
yarn run v1.22.22
$ react-app-rewired --max_old_space_size=4096 build
Creating an optimized production build...
Browserslist: caniuse-lite is outdated. Please run:
  npx update-browserslist-db@latest
  Why you should do it regularly: https://github.com/browserslist/update-db#readme
Failed to compile.

./node_modules/@softwarefactory-project/re-ansi/src/Ansi.mjs
Can't import the named export 'jsx' from non EcmaScript module (only default export is available)
TristanCacqueray commented 1 month ago

How this works is beyond my comprehension, I guess the culprit is lack of esmodule support from node/create-react-app/jest. I'm not sure why adding the transformIgnorePatterns tripped the d3 usage, but by adding an --transformIgnorePatterns 'node_modules/d3/' finally makes the test suite pass.

For the reference, here is the change that integrates the latest re-ansi library in the downstream javascript project: https://review.opendev.org/c/zuul/zuul/+/921474

cometkim commented 1 month ago

Jest has supported ESM since v29.7, but it is still experimental and never stabilized.

The latest release of create-react-app was 2 years ago, with Jest v27. Seriously, it's a graveyard technology.

I'd recommend migrating to a modern toolchain like Vite + Vitest.

fhammerschmidt commented 1 month ago

Yes, I made the same suggestion as well: https://github.com/rescript-lang/rescript-compiler/issues/6754#issuecomment-2110372226 It was famously also mentioned at the last React Conf: https://youtu.be/T8TZQ6k4SLE?t=5825

cometkim commented 1 month ago

compiler libraries should not be embedded by bundlers. It works since it's side-effect-free modules, but it makes a lot of duplication anyway.

@rescript/core and @rescript/react packages should also be marked as external and as dependencies or peerDependencies.

So the minimal configuration would be:

{
  "type": "module",
  "main": "./dist/Ansi.res.js",
  "exports": {
    ".": {
      "import": "./dist/Ansi.res.js",
      "require": "./dist/Ansi.res.cjs"
    },
    "./package.json": "./package.json"
  },
  "scripts": {
    "test": "npm run build && node tests/Spec.res.mjs",
    "bundle": "nanobundle build --clean --out-dir dist"
  },
  "peerDependencies": {
    "@rescript/core": "^1.0.0",
    "@rescript/react": "^0.12.0",
    "react": "^17.0.0 || ^18.0.0 || ^19.0.0-0",
    "react-dom": "^17.0.0 || ^18.0.0 || ^19.0.0-0",
    "rescript": "^11.0.0"
  },
  "devDependencies": {
    "@rescript/core": "^1.3.0",
    "@rescript/react": "^0.12.1",
    "nanobundle": "^2.1.0",
    "react": "^18.3.1",
    "react-dom": "^18.3.1",
    "rescript": "^11.1.0"
  }
}

Unfortunately, this doesn't work without an importmap. The @rescript/core module, unlike rescript/lib, doesn't have importmap-friendly prefix paths. So we need to list all used modules manually.

{
  "imports": {
    "@rescript/core/src/Core__List.res.js": {
      "import": "@rescript/core/src/Core__List.res.js",
      "require": "@rescript/core/src/Core__List.res.cjs"
    },
    "@rescript/core/src/Core__Option.res.js": {
      "import": "@rescript/core/src/Core__Option.res.js",
      "require": "@rescript/core/src/Core__Option.res.cjs"
    },
    "rescript/lib/es6/": {
      "import": "rescript/lib/es6/",
      "require": "rescript/lib/js/"
    }
  },
}

This also doesn't work. Because the @rescript/core module never generated *.cjs files yet. The only workaround here is to have the @rescript/core package include only pre-built *.cjs files, and let the user generate *.mjs on demand, which could be transpiled into *.cjs again by bundlers.

Honestly this is a nightmare to library authors and will never be fixed until the dual package support is implemented.

TristanCacqueray commented 1 month ago

@cometkim thank you for your suggestions and for your work to improve the library authoring experience.

I don't own the downstream app and migrating to Vitest just for this does not sound reasonable. I picked rescript to avoid dealing with such systems in the first place, so it's a bit unfortunate that regular scripts are no longer working for javascript user. Though I understand the need to move on to esmodules and I'm glad we can work around them with the transformIgnorePatterns trick.

As you can probably tell, I'm not familiar with the javascript stack, and it's surprising to read that create-react-app is already deprecated, which makes me wonder how long until Vite also become graveyard :)