-
Notifications
You must be signed in to change notification settings - Fork 22
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 VERSION_NAME in Banner + do a full extension reload #618
Conversation
src/background.ts
Outdated
localStorage.setItem("dev:last-version", version_name); | ||
} | ||
}); | ||
} |
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.
Want me to move it to a new file?
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.
Yes, I think it's helpful to keep generic functionality in separate modules.
As we collect more generic functionality, we can decide to upstream into dependencies (e.g., web-ext) or create a new module so others can benefit from it
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 added a /development/ folder so that we separate dev code from production code
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.
Approved - see comment on moving reload code to it's own module and then go ahead and merge
There's also a merge conflict with the GAPI PR I merged in
|
||
// In Chrome, `web-ext run` reloads the extension without reloading the manifest. | ||
// This forces a full reload if the version hasn't changed since the last run. | ||
if (process.env.ENVIRONMENT === "development" && isChrome) { |
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.
@fregante As per our side-effect free module initialization discussion, I think this should be wrapped in a method, e.g., installDevAutoReloadHook
or something similar
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 that makes it harder to treeshake. Given it's a dev-only background script, I don’t think that discussion applies here (it was mostly to avoid running unnecessary code when the content script was injected multiple times).
Also we want this code to be executed as soon as possible instead of after everything where the other initialization functions are
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 code is small enough that I don't think tree-shaking is a big concern.
I'm going to merge in now though since I'm having some issues with background page reloading during development. I want to see if this solves the issue
Fixes:
web-ext run
did not fully reload the extension (mozilla/web-ext#2277) in ChromeI added some hackish code that forces a reload if a previous update/install matches the current version. The only drawback is that clicking the reload button now causes a second reload within 500ms.