Skip to content

Support CJS file extensions#12021

Open
internettrans wants to merge 4 commits into
react:mainfrom
internettrans:cjs-support
Open

Support CJS file extensions#12021
internettrans wants to merge 4 commits into
react:mainfrom
internettrans:cjs-support

Conversation

@internettrans
Copy link
Copy Markdown

@internettrans internettrans commented Feb 3, 2022

Currently CRA treats .cjs files as asset/resource files rather than javascript files. See https://nodejs.org/api/packages.html#determining-module-system for explanation of how the .cjs file extension is treated by NodeJS.

I wasn't able to add tests to this PR because all of the tests failed epicly when I ran them locally, after following the instructions in CONTRIBUTING.md and swapping between Node 14, 16, and 17. I manually tested this change on a local create-react-app project to verify that CJS files are now processed as javascript. I expect the lack of tests will make this PR not mergeable, so any guidance on how to fix the many many errors that occur while running npx jest test/ --watchAll is appreciated.

@jenseng
Copy link
Copy Markdown

jenseng commented Feb 3, 2022

👍 would be great to see this merged, quite a few libraries use .cjs (e.g. @apollo/client), so the current webpack config can cause weird module import failures depending on what you're doing. The alternative is to use something like craco to monkey-patch the webpack config, but that's obviously not ideal.

@paulhklam1122
Copy link
Copy Markdown

@jenseng Second that! This is also affecting the very popular file uploader package uppy and all its sub-packages (@uppy/react, @uppy/dashboard, @uppy/aws-s3, @uppy/drag-drop) to name a few.

@felschr
Copy link
Copy Markdown

felschr commented Mar 29, 2022

This PR successfully fixes the issue for my project.
Shouldn't this be prioritised, @iansu? Looks like a common issue.

@ndrabins
Copy link
Copy Markdown

ndrabins commented Apr 3, 2022

Would love if this gets merged in as it solves issues with a lot of libs

@nyan-left
Copy link
Copy Markdown

nyan-left commented Apr 25, 2022

Would be great to have this merged in.

In the meantime, it's possible to continue using .cjs modules with react-app-rewired:

// config-overrides.js
module.exports = {
  webpack: function (config, env) {
    config.module.rules = config.module.rules.map(rule => {
      if (rule.oneOf instanceof Array) {
        rule.oneOf[rule.oneOf.length - 1].exclude = [/\.(js|mjs|jsx|cjs|ts|tsx)$/, /\.html$/, /\.json$/];
      }
      return rule;
    });
    return config;
  },
}

@Ionut-Milas
Copy link
Copy Markdown

Guys, any update on this ?

@aaronovz1
Copy link
Copy Markdown

This solved our issue in solana-labs/oyster#538 - it would be great to have this merged! 🙏

@weidonglian
Copy link
Copy Markdown

Any update? This is a blocker!

@zxl777
Copy link
Copy Markdown

zxl777 commented Jun 4, 2022

Looking forward to someone finally fixing this.

@nyatskiv
Copy link
Copy Markdown

We need this as well and it's really important for our team, @UPPY doesn't work without a .cjs support and potentially @apollo/client in the future. Please review this and merge.

@nigelheap
Copy link
Copy Markdown

Hey @nyatskiv
I got uppy working by doing the following as a stopgap before they merge this

Install these packages

npm i react-app-rewired --save-dev
npm i customize-cra  --save-dev

Update your build and start scripts

  "scripts": {
    "start": "react-app-rewired start",
    "build": "react-app-rewired build",
    "test": "react-app-rewired test",
  }

Create a config-overrides.js file with the following contents

//@ts-ignore
const {
  override,
  addWebpackModuleRule,
  setWebpackTarget,
} = require("customize-cra");

module.exports = override(
  addWebpackModuleRule({
    test: /\.(cjs)$/,
    exclude: /@babel(?:\/|\\{1,2})runtime/,
    loader: require.resolve("babel-loader"),
    options: {
      babelrc: false,
      configFile: false,
      compact: false,
      presets: [
        [
          require.resolve("babel-preset-react-app/dependencies"),
          { helpers: true },
        ],
      ],
      cacheDirectory: true,
      cacheCompression: false,
      sourceMaps: true,
      inputSourceMap: true,
    },
  })
);

@vezwork
Copy link
Copy Markdown

vezwork commented Sep 7, 2022

+1 Hoping this will be reviewed and merged. This fixes an issue I'm having and I believe this would fix #12155.

arielsegura added a commit to nation-io/solana-dao-sdk that referenced this pull request Oct 6, 2022
* use spl-governance serializers

* fix demo app

setting GENERATE_SOURCEMAP=false due to react/create-react-app#11752

also using react-app-rewired due to react/create-react-app#12021 and solana-labs/oyster#538
@rcbevans
Copy link
Copy Markdown

rcbevans commented Dec 6, 2022

    config.module.rules = config.module.rules.map(rule => {
      if (rule.oneOf instanceof Array) {
        rule.oneOf[rule.oneOf.length - 1].exclude = [/\.(js|mjs|jsx|cjs|ts|tsx)$/, /\.html$/, /\.json$/];
      }
      return rule;
    });

You are my new favorite internet stranger @nyan-left! I've spent all afternoon fighting with this trying to figure out why my dependency importing axios wasn't working!

Thanks a lot, hopefully this can get merged in to fix it properly ASAP.

@deivamagalhaes
Copy link
Copy Markdown

I also would love for this to be reviewed and merged. 🙏

@tmilar
Copy link
Copy Markdown

tmilar commented Feb 28, 2023

    config.module.rules = config.module.rules.map(rule => {
      if (rule.oneOf instanceof Array) {
        rule.oneOf[rule.oneOf.length - 1].exclude = [/\.(js|mjs|jsx|cjs|ts|tsx)$/, /\.html$/, /\.json$/];
      }
      return rule;
    });

You are my new favorite internet stranger @nyan-left! I've spent all afternoon fighting with this trying to figure out why my dependency importing axios wasn't working!

Thanks a lot, hopefully this can get merged in to fix it properly ASAP.

Same here, importing axios@1.3.3 as require('axios') inside a dependency is not working. It's returning some path string instead of the actual module, causing axios_1.default to be undefined.

Screen Shot 2023-02-28 at 13 29 39

I could work around this by adding CRACO to my project, with the configuration suggested here: #11889 (comment).

Is there any estimated ETA for merging this PR, so we can just use latest react-scripts and axios versions out of the box?

@laustdeleuran
Copy link
Copy Markdown

laustdeleuran commented Mar 9, 2023

This PR has been kicking around for a year, and it clearly solves a known issue. What's blocking the progress here? 😖

It, and several other PRs doing the same thing, seem like they would effectively solve #12700.

@maartenverbaarschot
Copy link
Copy Markdown

Ran into this issue today, I'm glad I was able to eventually find this PR and learn about the root cause + workarounds. But like the previous commenter, I'm also wondering what is blocking the PR for so long. This seems like a good solution for an issue that has been reported multiple times by now.

@nigelheap
Copy link
Copy Markdown

nigelheap commented Mar 20, 2023

I think we need to give up on create react app, it looks like they are not supporting it anymore with the new docs anyway.

If you have a look at my comment from 9 months ago there is a way to use rewire to solve the issue

@laustdeleuran
Copy link
Copy Markdown

it looks like they are not supporting it anymore with the new docs anyway.

@nigelheap, can you expand on what you mean? Or link to where in the docs it says that they are not supporting (and what they're note supporting)?

@nigelheap
Copy link
Copy Markdown

@nigelheap, can you expand on what you mean? Or link to where in the docs it says that they are not supporting (and what they're note supporting)?

@laustdeleuran The new react docs don't mention using create react app.
https://react.dev/learn/start-a-new-react-project basically says to use a framework or use Vite or Percel.

Specific part about not using a framework https://react.dev/learn/start-a-new-react-project#can-i-use-react-without-a-framework

@TinyCamera
Copy link
Copy Markdown

TinyCamera commented Jul 8, 2023

Hello from 2023, burned a few hours on this frustrating issue! Any chance of getting this merged?

@deivamagalhaes
Copy link
Copy Markdown

I'm also having this issue.

@djejaquino
Copy link
Copy Markdown

@zpao sorry for the direct hit. But can you help merging this? Thank you

@javaspeak
Copy link
Copy Markdown

It is sad that this has not been sorted after 1 year of collective frustration - what is up?

@hugocostadev
Copy link
Copy Markdown

I'm still hopping this gets merge :/

Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 4, 2024
Summary:
Known issue with create-react-app. As more dependencies use these files, will need this support.

Main issue now is that the latest chronik-client won't work, since it uses the latest version of axios, which uses cjs files.

Cashtab will work with the latest version of chronik-client with this webpack config.

I can wait and add them together so there is a test plan, but imo it makes sense to keep this as an isolated change.

This is more or less a backport of (still unmerged) react/create-react-app#12021

Cashtab was initially a create-react-app app, but has been ejected and customized to support crypto dependencies.

Test Plan: `npm test`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D15077
@101arrowz
Copy link
Copy Markdown

This issue is still breaking otherwise correctly written packages used within CRA projects (e.g. 101arrowz/fflate#193). .cjs has been part of Node's module resolution system for a few years now, so I'd have hoped CRA would support it. Is there anything else that needs to be done for this to be merged?

@nyan-left
Copy link
Copy Markdown

My take here is that CRA is now abandonware and we should either eject, or migrate to other providers like vite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.