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

Fix language files for Node+ESM #1377

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from
Open

Fix language files for Node+ESM #1377

wants to merge 24 commits into from

Conversation

sequba
Copy link
Contributor

@sequba sequba commented Feb 15, 2024

Context

  • changed babel configuration to produce the ESM build with .mjs extensions and adjust the import and export statements in the output (custom babel plugin)
  • added exports property to package.json that lists all valid ways of importing HyperFormula
  • added the migration guide

Useful reading about similar solutions:

Related issues:

Fixes #1344

New import paths for the language files

Module system valid import paths in HF 2.x valid import paths in HF 3.0.0
ES modules
  • hyperformula/es/i18n/languages/
  • hyperformula/es/i18n/languages/frFR
  • hyperformula/es/i18n/languages/frFR,js
  • hyperformula/i18n/languages/
  • hyperformula/i18n/languages/frFR
  • hyperformula/es/i18n/languages/ (legacy)
  • hyperformula/es/i18n/languages/frFR (legacy)
  • hyperformula/es/i18n/languages/frFR,js (legacy)
CommonJS
  • hyperformula/commonjs/i18n/languages/
  • hyperformula/commonjs/i18n/languages/frFR
  • hyperformula/commonjs/i18n/languages/frFR.js
  • hyperformula/i18n/languages/
  • hyperformula/i18n/languages/frFR
  • hyperformula/commonjs/i18n/languages/ (legacy)
  • hyperformula/commonjs/i18n/languages/frFR (legacy)
  • hyperformula/commonjs/i18n/languages/frFR.js (legacy)
UMD hyperformula/dist/languages/ hyperformula/dist/languages/

Breaking change for projects that use Typescript, Angular <=17, or Webpack <=4

Angular

Angular version supported by Angular legacy import paths new import paths
18 yes
17 yes ⚠️works after configuring moduleResolution ⚠️works after configuring moduleResolution
16 with Typescript 5 until 2024-11-08 ⚠️works after configuring moduleResolution ⚠️works after configuring moduleResolution
16 with Typescript 4 until 2024-11-08 ⚠️works after upgrading to typescript 5 and configuring moduleResolution ⚠️works after upgrading to typescript 5 and configuring moduleResolution
15 no

Typescript

Typescript version legacy import paths new import paths
5 ⚠️works after configuring moduleResolution ⚠️works after configuring moduleResolution
4 ⚠️works after configuring moduleResolution ⚠️works after configuring moduleResolution

Webpack

Webpack version legacy import paths new import paths
5
5 + babel
4 ⚠️works after configuring resolver-default
4 + babel ⚠️works after configuring resolver-default

Webpack 4 has 2 separate issues:

  1. It doesn't know how to handle mjs files.
  2. It doesn't support exports property to package.json.

For the former issue, there is a workaround. It's enough to configure a rule for mjs files. But it seems that there is no way to make it understand the exports property. For that reason, Webpack 4 can work with HyperFormula 3.0.0, but only with import paths that match the actual paths (ignoring the exports property that provides the artificial paths).

Further reading:

Parcel

Parcel version legacy import paths new import paths
<2.9.0
>=2.9.0 ⚠️works after configuring resolver-default

Further reading:

Testing

Test importing HF in various setups

  • ESM in node project
  • ESM in react project (with Typescript)
  • ESM in angular 18 project -> works with default configuration
  • ESM in angular 17 project -> works after configuring moduleResolution
  • ESM in angular 16 project (typescript 5) -> works after configuring moduleResolution
  • ESM in angular 16 project (typescript 4) -> works after upgrading to typescript 5 and configuring moduleResolution
  • ESM in webpack 5 project
  • ESM in webpack 5 project + babel
  • ESM in webpack 4 project -> works only with legacy paths and requires special configuration
  • ESM in webpack 4 project + babel -> works only with legacy paths and requires special configuration
  • ESM in webpack 5 project with Typescript 5 -> works after configuring moduleResolution
  • ESM in webpack 5 project with Typescript 4 -> works after configuring moduleResolution
  • ESM in parcel 2.11 project -> works after configuring resolver-default
  • ESM in parcel 2.8 project -> only legacy paths
  • CJS in node project
  • CJS in parcel 2.11 project
  • UMD in browser

Demo projects are available in the hyperformula-demos repo.

Tested by @budnix:

Vite ^5.1.4
Parcel2 ^2.0.0
Webpack5 ^5.8.0
Webpack4 ^4.42.0

Tested by @magierg

Webpack 5.9 + TS
React 18^ + TS
Angular 16.0.2 + TS 4.4.3
Angular 17.3.1 + TS 5.2
Svelte 3.5.4 + vite 4
Vue 3.3.4 + vite 4.4.9

Further verification

  • go through the angular setup and polish the guide
  • go through the typescript 4 and 5 setup and polish the guide
  • go through the webpack 4 and 5 setup and polish the guide
  • go through the parcel setup and polish the guide
  • go through all the notes and comments in the PR and make sure the guide is complete
  • validate by publint
  • test with https://arethetypeswrong.github.io/
  • CR
  • (@magierg) test the migration guide: create new projects and set them up using the instructions from the guide
  • (@magierg) final QA approval

Types of changes

  • Breaking change (a fix or a feature because of which an existing functionality doesn't work as expected anymore)
  • New feature or improvement (a non-breaking change that adds functionality)
  • Bug fix (a non-breaking change that fixes an issue)
  • Additional language file, or a change to an existing language file (translations)
  • Change to the documentation

Checklist:

  • I have reviewed the guidelines about Contributing to HyperFormula and I confirm that my code follows the code style of this project.
  • I have signed the Contributor License Agreement.
  • My change is compliant with the OpenDocument standard.
  • My change is compatible with Microsoft Excel.
  • My change is compatible with Google Sheets.
  • I described my changes in the CHANGELOG.md file.
  • My changes require a documentation update.
  • My changes require a migration guide.

@sequba sequba self-assigned this Feb 15, 2024
@sequba sequba linked an issue Feb 15, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Feb 15, 2024

Performance comparison of head (152b0dd) vs base (67bae9e)

                                     testName |   base |   head | change
------------------------------------------------------------------------
                                      Sheet A | 504.08 | 500.26 | -0.76%
                                      Sheet B | 160.55 |  159.1 | -0.90%
                                      Sheet T | 140.77 | 140.28 | -0.35%
                                Column ranges | 502.13 | 503.84 | +0.34%
Sheet A:  change value, add/remove row/column |  13.83 |  14.21 | +2.75%
 Sheet B: change value, add/remove row/column |  122.3 | 118.76 | -2.89%
                   Column ranges - add column | 148.48 | 151.33 | +1.92%
                Column ranges - without batch | 436.49 | 444.19 | +1.76%
                        Column ranges - batch | 110.39 | 109.99 | -0.36%

@sequba sequba marked this pull request as ready for review February 21, 2024 14:59
@sequba sequba requested a review from budnix February 21, 2024 15:08
Copy link
Member

@budnix budnix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the dist files in some more popular bundlers and here are the results:

  • Vite ^5.1.4 - OK
  • Parcel2 ^2.0.0 - Does not support "exports" so the language file has to be imported with /es/ or /commonjs/ prefixes;
  • Webpack5 ^5.8.0 - OK
  • Webpack4 ^4.42.0 - I getting an error. Seems the webpack has the problem with importing values from "chevrotain".
// /node_modules/hyperformula/es/parser/FormulaParser.mjs
import { EmbeddedActionsParser, EMPTY_ALT, Lexer, tokenMatcher } from 'chevrotain';
// EmbeddedActionsParser, EMPTY_ALT and others are missing
ERROR in ./node_modules/hyperformula/es/parser/FormulaParser.mjs 689:125-137
Can't import the named export 'tokenMatcher' from non EcmaScript module (only default export is available)
 @ ./node_modules/hyperformula/es/parser/index.mjs
 @ ./node_modules/hyperformula/es/ArraySize.mjs
 @ ./node_modules/hyperformula/es/index.mjs
 @ ./src/index.js

ERROR in ./node_modules/hyperformula/es/parser/FormulaParser.mjs 708:29-41
Can't import the named export 'tokenMatcher' from non EcmaScript module (only default export is available)
 @ ./node_modules/hyperformula/es/parser/index.mjs
 @ ./node_modules/hyperformula/es/ArraySize.mjs
 @ ./node_modules/hyperformula/es/index.mjs
 @ ./src/index.js

package.json Outdated Show resolved Hide resolved
@sequba
Copy link
Contributor Author

sequba commented Feb 29, 2024

  • Webpack4 ^4.42.0 - I getting an error. Seems the webpack has the problem with importing values from "chevrotain".
// /node_modules/hyperformula/es/parser/FormulaParser.mjs
import { EmbeddedActionsParser, EMPTY_ALT, Lexer, tokenMatcher } from 'chevrotain';
// EmbeddedActionsParser, EMPTY_ALT and others are missing
ERROR in ./node_modules/hyperformula/es/parser/FormulaParser.mjs 689:125-137
Can't import the named export 'tokenMatcher' from non EcmaScript module (only default export is available)
 @ ./node_modules/hyperformula/es/parser/index.mjs
 @ ./node_modules/hyperformula/es/ArraySize.mjs
 @ ./node_modules/hyperformula/es/index.mjs
 @ ./src/index.js

ERROR in ./node_modules/hyperformula/es/parser/FormulaParser.mjs 708:29-41
Can't import the named export 'tokenMatcher' from non EcmaScript module (only default export is available)
 @ ./node_modules/hyperformula/es/parser/index.mjs
 @ ./node_modules/hyperformula/es/ArraySize.mjs
 @ ./node_modules/hyperformula/es/index.mjs
 @ ./src/index.js

I found out that webpack 4 does not support exports, and there seems to be no easy workaround for that. By tweaking the webpack configuration, I managed to make the project import HyperFormula correctly using the legacy paths. Working demo.

@sequba sequba requested a review from budnix February 29, 2024 15:52
@sequba sequba added the Breaking Change Will need a major release label Mar 5, 2024
@sequba sequba changed the title Fix language files for Node+ESM [DO NOT MERGE] Fix language files for Node+ESM Mar 5, 2024
@sequba sequba changed the title [DO NOT MERGE] Fix language files for Node+ESM [DO NOT MERGE!] Fix language files for Node+ESM Mar 5, 2024
@magierg
Copy link
Contributor

magierg commented Mar 27, 2024

I have retested all the demos available on https://github.com/handsontable/hyperformula-demos/tree/import-demos
image

plus the bundlers mentioned by @budnix getting the same results.

On top of the above I have tested:

  • Webpack 5.9 + TS - tsconfig.js setting change required:
"module": "node16",
"moduleResolution": "node16",
  • React 18^ + TS - same tsconfig.js change required
  • Angular 16.0.2 + TS 4.4.3 - @sequba to investigate tsconfig changes required
  • Angular 17.3.1 + TS 5.2 - same issue with TS
  • Svelte 3.5.4 + vite 4 - getting this error - might be user error - @sequba to investigate
Error: Language already registered.
    at HyperFormula.registerLanguage (/Users/magierg/repos/hyperformula-demos/svelte-demo/node_modules/hyperformula/commonjs/HyperFormula.js:287:13)
    at Hyperformula.svelte:18:15
    at Object.$$render (/node_modules/svelte/internal/index.mjs:1892:22)
    at eval (/src/routes/+page.svelte:24:148)
    at Object.$$render (/node_modules/svelte/internal/index.mjs:1892:22)
    at Object.default (root.svelte:41:38)
    at eval (/node_modules/@sveltejs/kit/src/runtime/components/layout.svelte:8:41)
    at Object.$$render (/node_modules/svelte/internal/index.mjs:1892:22)
    at root.svelte:40:37
    at $$render (/node_modules/svelte/internal/index.mjs:1892:22)
  • Vue 3.3.4 + vite 4.4.9 - OK

@sequba
Copy link
Contributor Author

sequba commented Apr 12, 2024

TODO: verify it with the code example in issue #1143

@sequba
Copy link
Contributor Author

sequba commented Apr 25, 2024

Svelte 3.5.4 + vite 4 - getting this error - might be user error - @sequba to investigate

It works correctly. The issue you reported was caused by running the setup code twice. I created a demo for svelte: https://github.com/handsontable/hyperformula-demos/tree/import-demos/import-demo-esm-svelte

Angular case is still to be verified.

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.36%. Comparing base (e32e132) to head (0cd4e05).
Report is 33 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1377   +/-   ##
========================================
  Coverage    97.36%   97.36%           
========================================
  Files          169      169           
  Lines        14421    14421           
  Branches      3021     3021           
========================================
  Hits         14041    14041           
  Misses         380      380           

@sequba
Copy link
Contributor Author

sequba commented Aug 8, 2024

Angular 16.0.2 + TS 4.4.3 - @sequba to investigate tsconfig changes required
Angular 17.3.1 + TS 5.2 - same issue with TS

Angular issue can be resolved by setting "moduleResolution": "bundler" in tsconfig.json. Tested with Angular 16, 17 and 18 and Typescript 5. Demo

Unfortunately, I couldn't make combination Angular + Typescript 4 work. I'm afraid we need to ask our clients to upgrade to Typescript 5 if they want to use HyperFormula 3.x with Angular.

@sequba sequba requested a review from budnix October 31, 2024 14:34
@sequba sequba changed the title [DO NOT MERGE!] Fix language files for Node+ESM Fix language files for Node+ESM Nov 5, 2024
Copy link
Member

@budnix budnix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👌

@magierg
Copy link
Contributor

magierg commented Nov 8, 2024

I have retested the previously mentioned configurations and performed the migration steps - found out that the @babel/core is not required - this has been already addressed by @sequba

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Will need a major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Language files do not work with ES modules in node
3 participants