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

Bug: JS v4.4.1 breaks with webpack, and changing resolve.conditionNames configuration is not expected #91

Closed
tagliala opened this issue Feb 12, 2024 · 25 comments

Comments

@tagliala
Copy link
Contributor

tagliala commented Feb 12, 2024

Incoming reproducible test case

https://github.com/diowa/icare/actions/runs/7872274448/job/21477141544?pr=1650#step:10:43

Relevant change: https://github.com/diowa/icare/pull/1650/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R32

Note

Sorry about the not-so-reduced test case. There are two JS errors, the second (google maps key) is unrelated. We are interested in the first one for this issue

Description

ERROR
i18n_js__WEBPACK_IMPORTED_MODULE_0__.I18n is not a constructor
TypeError: i18n_js__WEBPACK_IMPORTED_MODULE_0__.I18n is not a constructor

How to reproduce

import { I18n } from 'i18n-js'
import translations from 'src/locales/translations.json'

const i18n = new I18n(translations)

What do you expect

Since this is a minor release, I was expected the same behaviour as before. Or at least an entry in the changelog and updated documentation

What happened instead

Breaks JavaScript functionality

Software:

  • Gem version: 4.2.3
  • Ruby version: 3.3.0

Full backtrace

  1) Application protects pages from guest users
     Failure/Error: raise error_messages if severe_errors.any?
     
     RuntimeError:
       There were 2 JavaScript errors:
     
       http://127.0.0.1:54260/packs-test/js/application.js 217:13 Uncaught TypeError: i18n_js__WEBPACK_IMPORTED_MODULE_0__.I18n is not a constructor
tagliala added a commit to diowa/icare that referenced this issue Feb 12, 2024
@tagliala tagliala changed the title Bug: JS v4.4.1 broke build with webpack(er) Bug: JS v4.4.1 breaks with webpack(er) Feb 12, 2024
@Martinocom-Switcho
Copy link

Same on React-Native 0.72, it won't compile.

@janroures
Copy link

any solution @Martinocom-Switcho ?

@janroures
Copy link

For now I downgraded to 4.1.1 and it works

@fnando
Copy link
Owner

fnando commented Feb 12, 2024

Can someone please post a sample app reproducing the issue?

@fnando
Copy link
Owner

fnando commented Feb 12, 2024

I can confirm the issue is that react-native seems to be taking the value for browser, rather than anything else. No idea what react-native is doing under hood though, so I'll try to investigate. In the meantime you can either pin the version to v4.3 or use something like patch-package to change node_modules/i18n-js/package.json, removing browser and exports.browser. I think react-native supports react-native as another import path. If that works, I'll add that to the list.

@fnando
Copy link
Owner

fnando commented Feb 12, 2024

@tagliala @janroures @Martinocom-Switcho I just published v4.4.2-beta.0 with the fix (adding a specific import for react-native). Can you please give it a try? You can installed with yarn add i18n-js@next.

@short-dsb
Copy link

4.4.2-alpha.0 fixes the React Native issue for me.

@fnando
Copy link
Owner

fnando commented Feb 12, 2024

@short-dsb thanks for reporting it back. I'm going to release 4.4.2 then.

@Gamote
Copy link

Gamote commented Feb 12, 2024

Same 4.4.2-alpha.0 works as expected. Thank you for you time put into fixing this issue @fnando.

@short-dsb
Copy link

Thanks for the quick fix, @fnando. 🙂

@fnando
Copy link
Owner

fnando commented Feb 12, 2024

Just released v4.4.2.

@fnando fnando closed this as completed Feb 12, 2024
@wooly
Copy link

wooly commented Feb 13, 2024

Hi @fnando, I don't think the issue is fixed for webpack with 4.4.2. We're still seeing the same error as reported by @tagliala on 4.4.2 as well:

Uncaught TypeError: i18n_js__WEBPACK_IMPORTED_MODULE_6___default() is not a constructor

@fnando
Copy link
Owner

fnando commented Feb 13, 2024

@wooly let me try to quickly set up a small project using webpack. Could you please share your config (or the most relevant pieces of it) that I should use?

@fnando
Copy link
Owner

fnando commented Feb 13, 2024

Got it working. Looks like you need to add the resolve.conditionNames. This is what I did:

/**
 * @type {import('webpack').Configuration}
 */
module.exports = {
  entry: "./index.js",
  mode: "production",
  target: "web",
  resolve: {
    conditionNames: ["import", "require", "module", "main"],
  },
  output: {
    filename: "output.js",
    path: __dirname,
  },
};

I'm going to update the readme file with this info.

webpack-i18n.zip

@jbanulso
Copy link

jbanulso commented Feb 13, 2024

I just wanted to mention that this is still an issue when using the latest version of Expo (v50) and bundling for web. I have tried customising the Metro config file as follows but then I got a different error: Uncaught TypeError: _interopRequireDefault is not a function.

// Learn more https://docs.expo.io/guides/customizing-metro
const { getDefaultConfig } = require('expo/metro-config');

/** @type {import('expo/metro-config').MetroConfig} */
const config = getDefaultConfig(__dirname);

config.resolver.unstable_enablePackageExports = true;
config.resolver.unstable_conditionsByPlatform['web'] = ['browser', 'import', 'require', 'module', 'main'];

module.exports = config;

@fnando
Copy link
Owner

fnando commented Feb 13, 2024

@jbanulso did you try v4.4.2? It adds react-native as an export, so you don't need to tweak metro's config at all. I just started a brand new project with it and it worked out of box.

I18nSampleExpo.zip

Btw, browser as the first condition is exactly the problem; it needs to be the last, so it should be ['import', 'require', 'module', 'main', 'browser'] instead. Just saw you're bundling for the web. I have no idea if browser would ever work, so maybe move it down the list and see what works best?

tagliala added a commit to tagliala/i18n-js-issue-91 that referenced this issue Feb 13, 2024
@tagliala
Copy link
Contributor Author

tagliala commented Feb 13, 2024

Hello, here it is a simple reproducible test case with webpack

https://github.com/tagliala/i18n-js-issue-91

Didn't bisect (it is quite difficult for me as a non JavaScript developer. Also tried yalc without success)

The support broke between 4.4.0 and 4.4.1 because of

"browser": "./dist/browser/index.js",

Removing this line or moving it below "import" allows webpack to work


It is this line: 4c4193a#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R33

tagliala added a commit to tagliala/i18n-js-issue-91 that referenced this issue Feb 13, 2024
@fnando
Copy link
Owner

fnando commented Feb 13, 2024

@tagliala your sample works if you create a file webpack.config.js with the following content.

/**
 * @type {import('webpack').Configuration}
 */
module.exports = {
  resolve: {
    conditionNames: ["import", "require", "module", "main"],
  },
};

Here's the whole thing in action:

$ cat webpack.config.js && webpack && node dist/main.js
/**
 * @type {import('webpack').Configuration}
 */
module.exports = {
  resolve: {
    conditionNames: ["import", "require", "module", "main"],
  },
};
asset main.js 69.2 KiB [emitted] [minimized] (name: main)
orphan modules 64.5 KiB [orphan] 25 modules
runtime modules 1010 bytes 5 modules
modules by path ./node_modules/lodash/*.js 153 KiB
  ./node_modules/lodash/get.js 884 bytes [built] [code generated]
  ./node_modules/lodash/has.js 757 bytes [built] [code generated]
  ./node_modules/lodash/merge.js 1.19 KiB [built] [code generated]
  ./node_modules/lodash/uniq.js 688 bytes [built] [code generated]
  ./node_modules/lodash/_baseGet.js 616 bytes [built] [code generated]
  ./node_modules/lodash/_baseHas.js 559 bytes [built] [code generated]
  ./node_modules/lodash/_hasPath.js 1.06 KiB [built] [code generated]
  ./node_modules/lodash/_baseMerge.js 1.3 KiB [built] [code generated]
  ./node_modules/lodash/_createAssigner.js 1.02 KiB [built] [code generated]
  + 192 modules
./src/index.js + 24 modules 64.6 KiB [built] [code generated]
./node_modules/bignumber.js/bignumber.js 87.6 KiB [built] [code generated]

WARNING in configuration
The 'mode' option has not been set, webpack will fallback to 'production' for this value.
Set 'mode' option to 'development' or 'production' to enable defaults for each environment.
You can also set it to 'none' to disable any default behavior. Learn more: https://webpack.js.org/configuration/mode/

webpack 5.90.1 compiled with 1 warning in 758 ms
Hello World

@tagliala
Copy link
Contributor Author

Thanks for your answer

I can confirm that the change in configuration allows the reduced test case to pass. However, in a real application, there are several other packages, and when trying the change there the build breaks on other packages (an example is font awesome stylesheets)

So I'm asking why this change is only needed by i18n-js, and the impact that will have this on all applications using i18n-js (I mean, change the configuration defaults for all applications with a specific reference to this issue)

Even if we are trying to move out from webpacker, I can see that a similar issue also happens with esbuild, where a change to the main fields is required

I've tried to understand https://webpack.js.org/guides/package-exports/ but it is unclear to me, so I have other questions:

  • what is browser export entry for?
  • can it be moved below import?
  • can we introduce webpack (and maybe esbuild) exports to make webpack (and esbuild) happy and do not attempt to use browser or require a potential breaking change in the configuration of all applications using i18n-js?

@scesbron
Copy link

I have a monorepo with i18n-js in a common package that is used by a webapp (webpack with craco) and a react-native app.

With latest version 4.4.2, it works well on react-native but for my web app I still have the error Uncaught TypeError: i18n_js__WEBPACK_IMPORTED_MODULE_6___default() is not a constructor.

I am not in the same context as the one of your example @fnando, I don't have webpack settings in my common package but only some webpacks params in my craco.config.js.

@tagliala tagliala changed the title Bug: JS v4.4.1 breaks with webpack(er) Bug: JS v4.4.1 breaks with webpack, and changing module.conditionNames configuration is not expected Feb 14, 2024
@tagliala tagliala changed the title Bug: JS v4.4.1 breaks with webpack, and changing module.conditionNames configuration is not expected Bug: JS v4.4.1 breaks with webpack, and changing resolve.conditionNames configuration is not expected Feb 14, 2024
@tagliala
Copy link
Contributor Author

tagliala commented Feb 14, 2024

Is it possible to reopen this issue and implement a solution that does not require to change resolve.conditionNames?

Since I cannot find a way to override exports, the only solution for me to upgrade would be to fork and add a specific export for webpack, something like this:

  "exports": {
    ".": {
+     "webpack": "./dist/import/index.js",
      "browser": "./dist/browser/index.js",
      "import": "./dist/import/index.js",

or invert import and browser. I'm struggling to understand what browser does here and if it is an issue to move it down the hierarchy below import, can I have a test case where "browser" is used?

@fnando
Copy link
Owner

fnando commented Feb 14, 2024

@tagliala I'm undoing this change. Can you please try v4.4.3-beta.0?

@tagliala
Copy link
Contributor Author

tagliala commented Feb 14, 2024

@fnando I confirm that v4.4.3-beta.0 works again

It is clear form the code because exports entry in package.json has been removed

From the reproducible test case above:

$ npm list && node dist/main.js
webpack-demo@1.0.0 ~/dev/tests/i18n-js-issue-91
├── i18n-js@4.4.3-beta.0
├── webpack-cli@5.1.4
└── webpack@5.90.1

Hello World

@scesbron
Copy link

On my side the latest v4.4.3-beta.0 works correctly in my monorepo for both react-native and react with webpack, thank you 👍

@tagliala
Copy link
Contributor Author

Thanks. Upgraded to 4.4.3, I can confirm the issue in the OP is fixed

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

9 participants