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

front: switch package type to module #10671

Merged
merged 7 commits into from
Feb 11, 2025
Merged

front: switch package type to module #10671

merged 7 commits into from
Feb 11, 2025

Conversation

emersion
Copy link
Member

@emersion emersion commented Feb 4, 2025

See individual commits.

Modules are the standard way to handle JavaScript imports. osrd-ui already uses modules everywhere, but osrd is lagging behind.

Modernize our configuration to use ECMAScript Modules instead of legacy CommonJS.

@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Feb 4, 2025
@emersion emersion force-pushed the emr/module branch 5 times, most recently from e5c2185 to 3f73aa6 Compare February 4, 2025 16:20
@emersion emersion force-pushed the emr/module branch 17 times, most recently from fb765b4 to aaf9e0f Compare February 6, 2025 11:04
@emersion emersion marked this pull request as ready for review February 6, 2025 12:41
@emersion emersion requested review from a team as code owners February 6, 2025 12:41
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Lgtm and tested (didn't launch the e2e tests)

@emersion emersion requested a review from Maymanaf February 6, 2025 15:09
Copy link
Contributor

@Maymanaf Maymanaf left a comment

Choose a reason for hiding this comment

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

Good job !
However, the test appears to be flaky when using the pdfjs-dist library
image

require() cannot be used with packages marked as modules. We want
to switch osrd to modules.

Signed-off-by: Simon Ser <[email protected]>
We'll soon change the default to ESM.

rtk-query requires config files to be CJS:
reduxjs/redux-toolkit#2437 (comment)

Signed-off-by: Simon Ser <[email protected]>
JSON imports cause issues with ESM.

Signed-off-by: Simon Ser <[email protected]>
When directly importing a file from a CommonJS package, the file
extension needs to be specified:
https://nodejs.org/api/esm.html#esm_mandatory_file_extensions

Signed-off-by: Simon Ser <[email protected]>
pdf-parse is not maintained anymore and is causing issues with
newer standards:
https://gitlab.com/autokent/pdf-parse/-/issues/38

Instead, directly use the official pdf.js package.

We need to fiddle with transform[5] which indicates when a line
break happens:
https://stackoverflow.com/questions/54645206/pdfjs-get-raw-text-from-pdf-with-correct-newline-withespace

The legacy build needs to be used in Nodejs environments:
https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support

Signed-off-by: Simon Ser <[email protected]>
Modules are the standard way to handle JavaScript imports. osrd-ui
already uses modules everywhere, but osrd is lagging behind.

Modernize our configuration to use ECMAScript Modules instead of
legacy CommonJS.

Signed-off-by: Simon Ser <[email protected]>
We now use ESM everywhere, we don't need to resolve CommonJS nor
AMD anymore.

The rule is enabled by default by import/recommended, so dropping
the line just uses the defaults instead of custom options.

Signed-off-by: Simon Ser <[email protected]>
Copy link
Contributor

@Maymanaf Maymanaf left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@emersion emersion added this pull request to the merge queue Feb 11, 2025
Merged via the queue into dev with commit a98e13c Feb 11, 2025
27 checks passed
@emersion emersion deleted the emr/module branch February 11, 2025 10:16
@emersion emersion self-assigned this Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:front Work on Standard OSRD Interface modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants