-
-
Notifications
You must be signed in to change notification settings - Fork 473
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: add option to enable/disable HMR (options.hmr
)
#264
Conversation
options.json
Outdated
@@ -22,6 +22,9 @@ | |||
}, | |||
"convertToAbsoluteUrls": { | |||
"type": "boolean" | |||
}, | |||
"disableHMR": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disableHMR
=> hmr || hot
(with default true
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
@@ -141,6 +141,7 @@ Styles are not added on `import/require()`, but instead on call to `use`/`ref`. | |||
|**`insertInto`**|`{String}`|`<head>`|Inserts `<style></style>` into the given position| | |||
|**`sourceMap`**|`{Boolean}`|`false`|Enable/Disable Sourcemaps| | |||
|**`convertToAbsoluteUrls`**|`{Boolean}`|`false`|Converts relative URLs to absolute urls, when source maps are enabled| | |||
|**`disableHMR`**|`{Boolean}`|`false`|Avoids outputting the unnecessary Hot Module Replacement code block, good for non local development distribution| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|**`hmr || hot`**|`{Boolean}`|`true`|Enable/disable Hot Module Replacement (HMR), if disabled no HMR Code will be added (good for non local development/distribution)|
Add it as the first item in the options table please (above base
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
@@ -344,6 +345,21 @@ If convertToAbsoluteUrls and sourceMaps are both enabled, relative urls will be | |||
} | |||
``` | |||
|
|||
### `disableHMR` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disableHMR
=> hmr || hot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
@@ -344,6 +345,21 @@ If convertToAbsoluteUrls and sourceMaps are both enabled, relative urls will be | |||
} | |||
``` | |||
|
|||
### `disableHMR` | |||
|
|||
When bundling for non development environment distribution, turn this option on to avoid outputting unnecessary Hot Module Replacement code block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reword this sentence please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
disableHMR
option to avoid outputting unnecessary H…options.hmr
)
@drawyan Please also add a test for this |
a7013ca
to
6d494cf
Compare
@michael-ciniawsky New patch set updated, sorry for the delay of the weekend. Many thanks for the review! |
Sorry, accidentally closed it. |
index.js
Outdated
@@ -17,6 +17,24 @@ module.exports.pitch = function (request) { | |||
|
|||
validateOptions(require('./options.json'), options, 'Style Loader') | |||
|
|||
options.hot = typeof options.hmr === 'undefined' ? options.hot : options.hmr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use only one option? Example, i think hmr
is better, because hot
property is part of hmr
(and can be misleading).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
index.js
Outdated
@@ -17,6 +17,24 @@ module.exports.pitch = function (request) { | |||
|
|||
validateOptions(require('./options.json'), options, 'Style Loader') | |||
|
|||
options.hot = typeof options.hmr === 'undefined' ? options.hot : options.hmr; | |||
|
|||
var hmrBlock = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmrBlock
=> hmr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
@@ -142,6 +143,28 @@ Styles are not added on `import/require()`, but instead on call to `use`/`ref`. | |||
|**`sourceMap`**|`{Boolean}`|`false`|Enable/Disable Sourcemaps| | |||
|**`convertToAbsoluteUrls`**|`{Boolean}`|`false`|Converts relative URLs to absolute urls, when source maps are enabled| | |||
|
|||
### `hmr || hot` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😛 ||
(OR) :D. Let's proceed with hot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Or |
OK, I'll go with |
1d94470
to
2a07baa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drawyan Thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good! Some remarks.
index.js
Outdated
" // When the module is disposed, remove the <style> tags", | ||
" module.hot.dispose(function() { update(); });", | ||
"}" | ||
options.hmr ? hmr : "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid breaking changes set by default to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh 😲 good catch. Yep it must be true
by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already handled by the options.json
's default
value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also my unit test covers the default case by not having any values for this option and it won't break because the ajv handles the default
value from options.json
correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options.hmr = options.hmr || true
(L19+) it shouldn't be mandatory to set this in webpack.config.js
and it would be a breaking change aswell then. And please remove the default: true
in case it causes a ValidationError
for
{
loader: 'style-loader',
options: {}
},
{
loader: 'style-loader',
options: {
other: option
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and updated.
README.md
Outdated
### `hmr` | ||
|
||
Enable/disable Hot Module Replacement (HMR), if disabled no HMR Code will be added. | ||
This could be used for non local development and distribution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think better to use production
, distribution
is misleading. distribution
can be source and minified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
index.js
Outdated
@@ -17,6 +17,22 @@ module.exports.pitch = function (request) { | |||
|
|||
validateOptions(require('./options.json'), options, 'Style Loader') | |||
|
|||
var hmr = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's return the name hmrBlock
. hmr
is option, hmrBlock
is code block for hmr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Block
doesn't add any semantic meaning, it's already indicated by {Array}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-ciniawsky hmrCode
or hmrSource
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just hmr
as there isn't anything to distinguish from (atm). Only options.hmr
(The Options) && hmr
(The Code), The fact that it's a block ({Array}
Type) and it is Source Code is already fulfilled 😛 , it just needs a name. hmrCode
is also fine, but it's not really needed imho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-ciniawsky I think better using different variable names, even if they are stored in different places, it makes the reading easier not to be misleading, also when using destructuring it helps to avoid accidentally overriding the variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just leave as is for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if, agreed. Still const { code } = hmr
with hmr
as the 'container/wrapper' then ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, hmr
being a wrapper seems overkill here, but I do agree that it shouldn't be hmr
to differentiate from the option's key, and this is why I put hmrBlock
to indicate it's a code block. But as @michael-ciniawsky pointed out block
being a bit confusing, I would say hmrCode
is my pick. Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drawyan
Sorry, we forget about this lines:
Line 25 in 97508ec
"if(module.hot) {", |
Line 37 in 4b6b70d
"if(module.hot) {", |
style-loader/lib/addStyleUrl.js
Line 33 in 4b6b70d
if (module.hot) { |
And tests
index.js
Outdated
@@ -16,6 +16,23 @@ module.exports.pitch = function (request) { | |||
var options = loaderUtils.getOptions(this) || {}; | |||
|
|||
validateOptions(require('./options.json'), options, 'Style Loader') | |||
options.hmr = typeof options.hmr === 'undefined' ? true : options.hmr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use short-circuit
here (options.hmr = options.hmr || true
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and space (\n
) [L18]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how options.hmr = options.hmr || true
is going to work. We'll never be able to set this flag to false
, right?
@evilebottnawi I've updated other files, but not sure how to test them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, maybe we can use mock for testing this?
@evilebottnawi Mocked and updated, please review again. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks!
When will this be merged and released? My project is actually dependent on this. Thanks! |
@drawyan I near future we release new version, thank you! |
This is really awesome! Thanks so much for this feature. :) |
Issues