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

Core platform update to Node version 18 #9

Merged
merged 17 commits into from
Jun 12, 2024
Merged

Conversation

Sakelun
Copy link
Collaborator

@Sakelun Sakelun commented Jun 4, 2024

This PR updates the core platform to Node JS 18 from Node JS 10 and also includes a required initial migration from Material UI v4 to v5.

Important: This work results in a stable system-level experience: the application will compile, run, serve content, and work in most respects. The user interface has some aesthetic or functional regressions that are being addressed in follow-up work.

WHAT'S NEW

Testing Instructions

Set-up

  1. Pull the dev-vertex/nodejs-18-upgrade branch
  2. Ensure you are using the correct node version. This project provides a .nvmrc file that specifies the recommended Node version, to use it (assuming you have nvm installed): nvm use. It may ask require you to install the version prior to use - this can be done with nvm install lts/hydrogen
  3. Do a first-pass installation of packages to support npm run clean ... scripts: npm ci
  4. Run the clean: npm run clean. Note: this will remove any current data but retain the dataset templates.
  5. Install the packages again: npm ci

You are now ready to test.

Known Issues

  • The Electron console does not render correctly. The application is still served and made available however.
  • There are visual and other inconvenient defects that have been introduced by this update that relate to the migration of Material UI from Version 4 to Version 5. This will be addressed in follow-up work.

Testing Instructions

Test the following scripts to confirm that they work as expected (compile, package, etc.) For the scripts that start/package the application, confirm the the application is available on the listed port and performs basic application functions.

  • npm run dev
  • npm run package
    • Linux (should run, but Electron console does not render correctly. See "Known Issues")
    • macOS
  • npm run electron (should run, but Electron console will not render correctly. See "Known Issues")
  • npm run start
  • npm run clean
  • npm run clean:all
  • npm run appsign
    • Linux (should run, but Electron console does not render correctly. See "Known Issues")
    • macOS
  • npm run app

Other Technical Notes

  • "hydrogen" is the Long Term Support name for the NodeJS 18 version. This was chosen to allow patch/fixes to NodeJS to be applied. The LTS versions should not include breaking changes.
  • Material UI version 5 adopted Emotion as the default CSS rendering engine but allows for the usage of other engines. For the purposes of getting the application to a running system, we are currently using styled components in lieu of JSS which was previously used.

@dsriseah
Copy link

Really minor note:

  • after running npm run clean:all test step, npm ci has to be inserted for npm run appsign

@dsriseah
Copy link

confirm it runs, with the exception of not testing the run appsign and run app because my Apple IDs aren't set up on this machine yet.

@Sakelun Sakelun changed the title DRAFT: Core platform update to Node version 18 Core platform update to Node version 18 Jun 11, 2024
@dsriseah dsriseah requested review from dsriseah and removed request for benloh June 12, 2024 18:06
Copy link

@dsriseah dsriseah left a comment

Choose a reason for hiding this comment

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

Impromptu review notes

package.json

  • package.json adds @emotion which is fine
  • the svg.draggable.js and panzoom.js are new additions, though I thought this worked before (I will have to check if we still have standalone vendor copies)
  • MUI5 update expected, and noting additional change to bootstrap version if that matters anywhere. Accepting.
  • lots of nice updates overall

MEMEstyles.jsx

  • use of ? optional chaining spotted! ES2020, check babel config to ensure it's supported
  • there are other interesting changes in default values, but nothing show-stopping to keep this PR from moving on

pmc-data InitializeModel()

This code was removed, not sure why

  // MONKEY PATCH graphlib so it doesn't use ancient lodash _.keys()
  // command, which converts numbers to string.
  m_graph.nodes = () => Object.keys(m_graph._nodes);

src/app-web/modules/svgjs-plugin-draggable.js

This is our vendor copy of svgjs-plugin-draggable that is duplicated in package.json. Need to check which one is actually being imported. We have our own copy because we needed to debug or add features and couldn't use the plugin as-is, iirc.

src/app-web/views/ViewMain/ToolsPanel.jsx

Shows some interesting examples of conversion from withStyles to styled

src/config/webpack.base.config.js

Noting removal of ancient webpack configuration style in the test for jpegs, pngs, gifs. Not sure if it actually affects working code or where, but noting it.

            use: [{ loader: 'file-loader?name=img/[name]__[hash:base64:5].[ext]' }],
            // note: webpack imported images probably are only in our source folder
            // doesn't cover static assets loaded at runtime (?)
            include: defaultInclude

This appears to be using the file-loader which might specific assets in an img folder that are directly imported as base64-encoded string. Not sure where this might be done. The replacement code is in a generator field:

            generator: {
              filename: 'img/[name]__[hash:base64:5][ext]'
            },

There's also one for font files. Not sure if this is actually anything.

in the console config, also noting changes to syntax in CopyWebpackPlugin

src/system/server-express.js

Possible Turbo360 related changes?

Copy link

@dsriseah dsriseah left a comment

Choose a reason for hiding this comment

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

I think this looks good to go for our purposes of having an updated package. A lot of work into updating all those components and little changes. Much appreciated!!! I've pulled and tested the branch yesterday, and the repo change to the README since then doesn't need me to rerun everything.

@dsriseah dsriseah merged commit b39f1e0 into dev Jun 12, 2024
@dsriseah dsriseah deleted the dev-vertex/nodejs-18-upgrade branch June 12, 2024 18:47
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.

npm run clean and npm run clean:all returns various errors Build process regressions
5 participants