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: upgrade NGE to v2.7.0 #8309

Merged
merged 1 commit into from
Aug 5, 2024
Merged

front: upgrade NGE to v2.7.0 #8309

merged 1 commit into from
Aug 5, 2024

Conversation

emersion
Copy link
Member

@emersion emersion commented Aug 1, 2024

Unfortunately this turned out to be a tad more involved than expected.

Let's get the easy stuff out of the way first:

  • We're now using the official NGE package from NPM, so we can drop the @osrd-project namespace! 🎉
  • Thanks to Louis we now have translations and don't need to become fluent in German anymore!
  • Upstream no longer creates one subdirectory per language, so we can drop "en/" from our import paths.
  • Upstream has added a proper fix for dark themes, so we can drop our sbb-light class that we applied on the iframe's <html> element.
  • For reasons detailed below, we need to use vite-plugin-static-copy, and this plugin requires ESM instead of CJS, so we need to add "type": "module" in our package.json 1. Hopefully this Doesn't Break Stuff™. Edit: oh, of course this caused havoc everywhere, so let's just turn on ESM specifically for vite.config.ts instead of the whole project for now. This is done by renaming it to vite.config.mts.

Now onto the real fun. The source of the issue is that NGE now dynamically loads i18n files on startup (e.g. the JSON file containing English translations). Unfortunately this doesn't play well with our bundler, vite. vite doesn't include the NGE translation files in the final build. vite looks at all of the imports, then flattens them inside a single directory with a modified filename. Since NGE source files are immutable assets one does not simply import the i18n files from the React NGE component to make them available to NGE: we need the i18n files to be placed in a well-known directory at build time with a predictable filename.

The nicest way to resolve this would be some kind of directory URL import:

import ngeBase from 'netzgrafik-frontend/dist/netzgrafik-frontend?url';

However vite doesn't support importing directories like so and it doesn't seem like a plugin exists for this.

We can instead instruct vite to copy over the NGE i18n files via a third-party plugin named vite-plugin-static-copy. We need to grab the root directory of the netzgrafik-frontend package somehow, we could hardcode the "mode_modules/" path but that would be quite fragile. Instead, we can leverage require.resolve() but we need to pass the full path to a nested file then compute its dirname because that function doesn't like directories.

Now that our i18n files are copied to a directory, we need to convince NGE to pick them up at the correct location. As-is, NGE tries to load them relative to the URL, which results in hilarious paths such as:

/operational-studies/projects/2/studies/1/scenarios/1/src_assets_i18n_en_json.js

Append a <base> tag to the <iframe> so that anything NGE tries to load is looked up in the special NGE directory we've copied the i18n files into.

The cherry on top is:

TypeError: __require.resolve is not a function

which happens because require.resolve() is only available in CJS, and we just switched to ESM for the sake of the new vite-plugin-static-copy dependency. This can be resolved with a bit of juggling with createRequire from 'node:module'.


Ideas welcome if you find a better way to fix this!

@emersion emersion requested a review from a team as a code owner August 1, 2024 15:38
@emersion emersion requested a review from louisgreiner August 1, 2024 15:40
@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 27.47%. Comparing base (34c0b8e) to head (1f77b3f).

Files Patch % Lines
.../operationalStudies/components/MacroEditor/NGE.tsx 0.00% 7 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #8309      +/-   ##
============================================
- Coverage     27.48%   27.47%   -0.02%     
  Complexity     2155     2155              
============================================
  Files          1316     1316              
  Lines        158162   158163       +1     
  Branches       3262     3262              
============================================
- Hits          43469    43451      -18     
- Misses       112710   112729      +19     
  Partials       1983     1983              
Flag Coverage Δ
core 75.53% <ø> (ø)
editoast 64.58% <ø> (-0.07%) ⬇️
front 10.45% <0.00%> (-0.01%) ⬇️
gateway 2.03% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 72.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@emersion emersion force-pushed the emr/nge-upgrade branch 2 times, most recently from 5e9bf49 to 510f415 Compare August 1, 2024 15:52
Copy link
Contributor

@louisgreiner louisgreiner left a comment

Choose a reason for hiding this comment

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

Thank you for the update and the fix, which i guess was not easy to find 😮

Unfortunately this turned out to be more involved than expected.

Let's get the easy stuff out of the way first:

- We're now using the official NGE package from NPM, so we can
  drop the @osrd-project namespace! 🎉
- Thanks to Louis we now have translations and don't need to
  become fluent in German anymore!
- Upstream no longer creates one subdirectory per language, so
  we can drop "en/" from our import paths.
- Upstream has added a proper fix for dark themes, so we can drop
  our sbb-light class that we applied on the iframe's <html>
  element.
- For reasons detailed below, we need to use vite-plugin-static-copy,
  and this plugin requires ESM instead of CJS, so we need to add
  "type": "module" in our package.json [1]. Hopefully this Doesn't
  Break Stuff™. Edit: oh, of course this caused havoc everywhere,
  so let's just turn on ESM specifically for vite.config.ts instead
  of the whole project for now. This is done by renaming it to
  vite.config.mts.

Now onto the real fun. The source of the issue is that NGE now
dynamically loads i18n files on startup (e.g. the JSON file
containing English translations). Unfortunately this doesn't play
well with our bundler, vite. vite doesn't include the NGE
translation files in the final build. vite looks at all of the
imports, then flattens them inside a single directory with a
modified filename. Since NGE source files are immutable assets
one does not simply import the i18n files from the React NGE
component to make them available to NGE: we need the i18n files to
be placed in a well-known directory at build time with a predictable
filename.

The nicest way to resolve this would be some kind of directory URL
import:

    import ngeBase from 'netzgrafik-frontend/dist/netzgrafik-frontend?url';

However vite doesn't support importing directories like so and it
doesn't seem like a plugin exists for this.

We can instead instruct vite to copy over the NGE i18n files via a
third-party plugin named vite-plugin-static-copy. We need to grab
the root directory of the netzgrafik-frontend package somehow,
we could hardcode the "mode_modules/" path but that would be quite
fragile. Instead, we can leverage require.resolve() but we need to
pass the full path to a nested file then compute its dirname because
that function doesn't like directories.

Now that our i18n files are copied to a directory, we need to
convince NGE to pick them up at the correct location. As-is,
NGE tries to load them relative to the URL, which results in
hilarious paths such as:

    /operational-studies/projects/2/studies/1/scenarios/1/src_assets_i18n_en_json.js

Append a <base> tag to the <iframe> so that anything NGE tries to
load is looked up in the special NGE directory we've copied the
i18n files into.

The cherry on top is:

    TypeError: __require.resolve is not a function

which happens because require.resolve() is only available in CJS,
and we just switched to ESM for the sake of the new
vite-plugin-static-copy dependency. This can be resolved with a bit
of juggling with createRequire from 'node:module'.

[1]: https://vitejs.dev/guide/troubleshooting.html#this-package-is-esm-only
@emersion emersion self-assigned this Aug 1, 2024
Copy link
Contributor

@Uriel-Sautron Uriel-Sautron left a comment

Choose a reason for hiding this comment

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

Thank you for all these solutions, LGTM (tested) !!!

Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

LGTM & tested ✅

Thanks for the explanations !

@emersion emersion added this pull request to the merge queue Aug 5, 2024
Merged via the queue into dev with commit 2d5d004 Aug 5, 2024
20 checks passed
@emersion emersion deleted the emr/nge-upgrade branch August 5, 2024 07:49
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

Successfully merging this pull request may close these issues.

5 participants