-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix background HMR by @tyn1998 #110
base: master
Are you sure you want to change the base?
Conversation
Fix/hmr background
Add organizeImports and eslint config, update Popup, cleanup
expecting for merger |
@@ -21,6 +21,7 @@ | |||
"css": ["content.styles.css"] | |||
} | |||
], | |||
"host_permissions": ["http://*/"], |
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 you please explain why is this needed?
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 host_permissions
are required if you want to inject something in a matching page, download from a matching URL, or observe via webRequest and similar APIs.
Most extensions will need it, not all of course, I spent a considerable amount of time trying to figure out why things were not working and that was one of the issues.
Could be added to the readme or removed, as you wish, personally, for a boilerplate, I would add all the possible fields in there with default values so new developers know what they can use or could be the cause of an issue.
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.
It might make sense to have the fields you may want (as in my case, including host_permissions
) commented out in the file, and in the uncommented code support the most basic and least permissive cases. It's an approach I've seen in other config files that would also work here, and also one that doesn't risk someone accidentally asking for permissions they don't need.
it seems the package author is not active |
good job |
notHotReload: ['background', 'contentScript', 'devtools'], | ||
custom: { | ||
notHMR: ['background', 'contentScript', 'devtools'], | ||
enableBackgroundAutoReload: true, // always true when "enableContentScriptsAutoReload" is set 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.
It might be nice to link somewhere in the documentation that explains this reasoning.
@lxieyang I know things come up (I've been there an OSS maintainer in the past). If you're not able to remain active, would you mind considering opening up shared responsibility to contributors who appear interested in helping to maintain the repo? |
@@ -33,7 +33,7 @@ if (fileSystem.existsSync(secretsPath)) { | |||
alias['secrets'] = secretsPath; | |||
} | |||
|
|||
var options = { | |||
let options = { |
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.
Nitpick. This doesn't need to be a let
. Setting it to const
is fine, even though you're updating something within the object.
As per this comment #40 (comment) I extracted the fix for the HMR, and did some minor updates
TODO: Add instructions as explained in #40 (comment)