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

feat(hmr): adding hot module reloading #334

Merged

Conversation

ScriptedAlchemy
Copy link
Contributor

@ScriptedAlchemy ScriptedAlchemy commented Jan 6, 2019

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Initial implementation copied over from extract-css-chunks-webpack-plugin

Adding Eslint config for loaders that are injected directly into the browser.

These should remain ES5

Breaking Changes

Additional Info

This is a work in progress, looking for feedback on how to implement the HMR part in a better way.
Once the is discussed, ill write the readme and commit it to this branch

This is the unification process of my work on extract-css-chunks

evilebottnawi and others added 3 commits January 6, 2019 00:02
Initial implementation copied over from extract-css-chunks-webpack-plugin

Adding Eslint config for loaders that are injected directly into the browser.

These should remain ES5
…odule-reloading

# Conflicts:
#	package-lock.json
@ScriptedAlchemy
Copy link
Contributor Author

@evilebottnawi ill slack you tomorrow to discuss this further in detail.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Can we add tests for this?

src/hmr/.eslintrc.js Outdated Show resolved Hide resolved
src/hmr/hotLoader.js Outdated Show resolved Hide resolved
src/hmr/hotLoader.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
- Remove `this.cacheable` from hotLoader function
- Remove `isHMR`, simplifying codebase
- Adding `.idea` to gitignore. This prevents JetBrains products config files
Migration of hotLoader from its own file, which is injected into webpack config.
Into the base loader of mini-css, instead of adding options to the plugin. Add options to the loader.
@ScriptedAlchemy
Copy link
Contributor Author

Still need to test this further, but have been able to simplify it to pass most options via loader options. Testing filename output..

@alexander-akait
Copy link
Member

@ScriptedAlchemy you can looks how we can tests loader in other repositories (example in css-loader)

String replace based on filename coming in
updating js hash and adding hmr test cases
@ScriptedAlchemy ScriptedAlchemy force-pushed the hot-module-reloading branch 3 times, most recently from a0f62dd to 923342c Compare January 27, 2019 22:26
removing hot loader to fix tests
using version 3 of normalize-url for node 6 compatibility
@ScriptedAlchemy
Copy link
Contributor Author

@evilebottnawi im getting an error on canery

TypeError: Cannot read property 'tap' of undefined

Got any ideas? its passing everything else

Adding es-checker to prevent any es6 in the hot loader
@ScriptedAlchemy
Copy link
Contributor Author

@evilebottnawi, ive gotten it to pass almost every test, but it seems to fail at node6-canary any ideas as to what is causing this??

@alexander-akait
Copy link
Member

@ScriptedAlchemy don't see on canary tests (it is for webpack@5), plugin has no compatibility with webpack@5 right now

package.json Outdated Show resolved Hide resolved
@ScriptedAlchemy
Copy link
Contributor Author

@evilebottnawi I've updated the branch

@alexander-akait
Copy link
Member

@ScriptedAlchemy Thanks

@alexander-akait
Copy link
Member

Today i fix CI problem, update deps and boilerplate for plugin and do release

@alexander-akait alexander-akait merged commit 4ed9c5a into webpack-contrib:master Apr 9, 2019
@opeologist
Copy link

when can we expect a version bump + npm publish for this feature? just waiting on that to migrate our implementation to this! thanks again

@ScriptedAlchemy
Copy link
Contributor Author

There’s a few things being finalized in another branch. Mostly chores needed for the project

@zackschuster
Copy link

is there a reason to use normalize-url@^2.0.1? i ask because it'd be nice to have normalize-url^3.0.0 to line up with postcss-normalize-url@~4.0.0 (aka "what optimize-css-assets-webpack-plugin is doing")

thanks for your hard work! 😄

@alexander-akait
Copy link
Member

normalize-url use modern ES syntax so we need setup babel for transpile modern ES syntax to ES5 for client, you can send a PR

@alexander-akait
Copy link
Member

/cc @ScriptedAlchemy we should solve this in next PR

@zackschuster
Copy link

@evilebottnawi thanks for the explanation, i'll try to PR if i have the time!

@ScriptedAlchemy
Copy link
Contributor Author

ScriptedAlchemy commented Apr 10, 2019

It uses ES6 stuff that causes problem on older node versions @zackschuster

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.

8 participants