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

Client-side hot reloading / HMR #630

Merged
merged 25 commits into from
May 27, 2022
Merged

Client-side hot reloading / HMR #630

merged 25 commits into from
May 27, 2022

Conversation

vmarta
Copy link
Contributor

@vmarta vmarta commented May 25, 2022

Description

This PR adds a new feature to the runtime SDK, the one that all front-end developers love and are used to using it for granted nowadays, which is CLIENT SIDE HOT RELOADING!

With client side hot reloading, developers can save their changes in code editor and see the changes reflected on the browser immediately, without refreshing the web page.

See it in action:

Screen.Recording.2022-05-25.at.5.00.33.PM.mov

Related PR #379

TODO:

  • file 3pp request

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • installed dependencies: @pmmmwh/react-refresh-webpack-plugin and react-refresh
  • add webpack plugin to the webpack configuration
  • update dev server to serve modules by using webpack hot middleware

How to Test-Drive This PR

  • npm start in typescript template or retail react app
  • see changes reflected on browser without refreshing
  • refresh the browser and verify that there is no error in the console ← meaning that both client and server bundles are in sync, and that the server-side hot reloading from before still works

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@vmarta vmarta added the wip Work in process label May 25, 2022
@kevinxh kevinxh marked this pull request as ready for review May 25, 2022 23:53
@kevinxh kevinxh requested a review from a team as a code owner May 25, 2022 23:53
@kevinxh kevinxh added ready for review PR is ready to be reviewed and removed wip Work in process labels May 25, 2022
kevinxh and others added 2 commits May 27, 2022 12:57
* add /__mrt/hmr and remove publicPath

* update loadable extractor

* remove debuggers

* remove getPublicPath

* fix src path

* update /__mrt/hmr endpoint

* use 501 for disabled endpoint

const rules = [
ruleForBabelLoader([require.resolve('react-refresh/babel')]),
...config.module.rules.slice(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems fragile... can we give the loader rules an id and replace by id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevinxh I've tried adding an id and it failed because not matching the expected config schema. That's why I end up with this instead.

Copy link
Collaborator

@kevinxh kevinxh May 27, 2022

Choose a reason for hiding this comment

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

This feels like the Builder class lacks some basic functionalities. We need to be able to manipulate the config in a safer manner. Imagine we have many extend function calls and each of them adds/removes different rules, it's gonna be hard to maintain the webpack config.

@@ -71,6 +71,7 @@
"save-credentials": "pwa-kit-dev save-credentials",
"start": "cross-env NODE_ICU_DATA=node_modules/full-icu pwa-kit-dev start",
"start:inspect": "npm run start -- --inspect",
"start:no-hmr": "npm run start -- --noHMR",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if we want to have this as an project OOTB npm script. because if this feature works well, i'd imagine developers should never turn if off, or on a very very very rare case.

@@ -206,6 +209,7 @@ const withChunking = (config) => {

const ruleForBabelLoader = (babelPlugins) => {
return {
id: 'javascript',
Copy link
Collaborator

Choose a reason for hiding this comment

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

javascript is the target language for this rule but I feel more natural to call this the babel-loader, I feel like babel-loader kinda implies it is js/ts and it is also a little bit more descriptive?

...config.module.rules.slice(1)
]
const newRule = ruleForBabelLoader([require.resolve('react-refresh/babel')])
const rules = findAndReplace(config.module.rules, (rule) => rule.id === 'javascript', newRule)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We discussed that in the future, we might expose functionality for end users to customize specific rules. Maybe via the Builder class. However, that's out side of scope for this PR.

Copy link
Collaborator

@kevinxh kevinxh left a comment

Choose a reason for hiding this comment

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

Very excited to ship this feature!

Copy link
Collaborator

@kevinxh kevinxh left a comment

Choose a reason for hiding this comment

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

don't forget to update changelog :)

@vmarta vmarta merged commit 0974a89 into develop May 27, 2022
@vmarta vmarta deleted the hot-reloading branch May 27, 2022 23:14
@kevinxh kevinxh mentioned this pull request Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants