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

add support for HMR #5013

Merged
merged 11 commits into from Jun 22, 2022
Merged

Conversation

TheTrio
Copy link
Contributor

@TheTrio TheTrio commented Jun 16, 2022

Addresses #5003 and #5005

This PR attempts to add support for HMR along with in memory mode for Webpack.

It sets up a proxy so that all requests to Rails under /assets are forwarded to the Webpack dev server. The dev server holds these files in memory instead of writing them to the disk - which should make things a bit faster.

I've also had to split up app.jsx into multiple parts to ensure that the Redux store remains persistent across hot reloads.

There are still some issues - for example, pages which load TinyMCE will not be hot reloaded - but for the most part, HMR should work as expected. TinyMCE should no longer causes any issues. See below for details.

How to Enable

To enable it, make sure your application.yml has the following options set

DISABLE_SENTRY: "true"
hot_loading: "true"

After that, you can run either yarn hot or yarn hot:no-lint to start the dev server. Then start the Rails server as usual.

Now you can edit the React code and have it reflected in the browser without needing a refresh. The progress(and any errors) can be viewed in the browser console.

hmr

If the code results in an error(runtime or compile time), its shown in the browser as an overlay.

error

Across these "refreshes", the Redux state stays persistent. However, if you edit one of the reducer files, the page will hard reload.

@ragesoss
Copy link
Member

Are the un-covered lines in application_helper.rb now dead code?

@TheTrio
Copy link
Contributor Author

TheTrio commented Jun 17, 2022

No, I don't think so. I think the reason they're not being executed now is because I've added an additional check for Rails.env.development.

I found it annoying to have to disable hot_loading everytime I wanted to run a test. So I made sure hot loading was enabled only when you're in development mode.

@TheTrio
Copy link
Contributor Author

TheTrio commented Jun 18, 2022

With this, TinyMCE is now being bundled with Webpack directly, instead of us specifying the file manually in the config file.

This means two things -

  1. HMR works since TinyMCE is now part of the dependency chain
  2. We no longer need to insert the script tag manually in the HTML.

The comments in app/assets/javascripts/components/common/text_area_input.jsx say that dynamic imports didn't work earlier. I am not really sure what changed - maybe Webpack 4 had some issues with it or maybe there was some conflicting config option.

Regardless, dynamic imports work as expected now.

@TheTrio TheTrio marked this pull request as ready for review June 18, 2022 14:30
@ragesoss
Copy link
Member

Cool!

The DISABLE_SENTRY bit is going to be very confusing. I suggest that you add Features.sentry?, use that in _head.html.haml, and make it disable Sentry JS if hot loading is enabled. That way, just changing the hot_loading value in application.yml will be sufficient to turn it on/off. (See app/presenters/features.rb)

On my computer, it takes about two seconds to compile a change, and about three seconds total before the changed module re-renders. This feels a bit slow. Do you know why it takes 2 seconds to compile a trivial change to a single file?

@TheTrio
Copy link
Contributor Author

TheTrio commented Jun 21, 2022

On my computer, it takes about two seconds to compile a change, and about three seconds total before the changed module re-renders. This feels a bit slow. Do you know why it takes 2 seconds to compile a trivial change to a single file?

Did you try yarn hot:no-lint? That should skip the linting and should be a bit faster. Also, subsequent changes should take less time as the cache builds up.

@ragesoss
Copy link
Member

On my computer, it takes about two seconds to compile a change, and about three seconds total before the changed module re-renders. This feels a bit slow. Do you know why it takes 2 seconds to compile a trivial change to a single file?

Did you try yarn hot:no-lint? That should skip the linting and should be a bit faster. Also, subsequent changes should take less time as the cache builds up.

Do you think there's a good reason to leave linting enabled by default for yarn hot?

Seems like it needs to warm up with a few recompiles when linting is disabled, but it gets the compile time down to about half a second, which makes HMR a lot more fun to use.

@TheTrio
Copy link
Contributor Author

TheTrio commented Jun 21, 2022

Do you think there's a good reason to leave linting enabled by default for yarn hot?

Not really. Linting is enabled for yarn start which is why I enabled it for yarn hot. Disabling it by default would give a better experience though. What do you think?

@ragesoss
Copy link
Member

yeah, I think disabling it for yarn hot makes sense.

@TheTrio
Copy link
Contributor Author

TheTrio commented Jun 21, 2022

Couple of changes:

  1. yarn hot will disable linting by default.
  2. Setting hot_loading to "true" in application.yml will be enough to enable HMR

lib/dev_server_proxy.rb Outdated Show resolved Hide resolved
This test currently does nothing useful since it passes despite several
404 errors.
@ragesoss ragesoss merged commit 80bf0d9 into WikiEducationFoundation:master Jun 22, 2022
@TheTrio TheTrio deleted the devServerMemory branch June 22, 2022 20:32
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.

None yet

2 participants