-
Notifications
You must be signed in to change notification settings - Fork 785
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(hmr): allow changes to component decorators when using HMR #5290
Conversation
This fixes an issue where HMR updates which changed arguments to the `@Component` decorator would crash the application because a `HostRef` for the component could not be located. This occurred because HMR updates caused a new version of the component's lazy bundle to be fetched, which in turn would bundle the `getHostRef` function and the corresponding `hostRefs` variable, which is declared here: https://github.com/ionic-team/stencil/blob/0041dc27b9deb45cfd1434a3d82d563793e56056/src/client/client-host-ref.ts#L18 Because the newly fetched lazy entry point for the updated component did not have access to the `hostRefs` variable which was in-scope in the _old_ bundle, the `getHostRef` function would always return `undefined`, on the first render after the HMR update, causing issues in functions like `setValue` which make use of a `HostRef`. In order to fix the issue we can persist the `hostRefs` map across HMR updates by storing a reference to it on the `window` object. We can additionally constrain this persistence to only occur when we're building with HMR, so that we don't clutter up the `window` object of unsuspecting Stencil users in the context of a production build. STENCIL-973
9ba51b0
to
2ead573
Compare
|
Path | Error Count |
---|---|
src/dev-server/index.ts | 37 |
src/mock-doc/serialize-node.ts | 36 |
src/dev-server/server-process.ts | 32 |
src/compiler/prerender/prerender-main.ts | 22 |
src/testing/puppeteer/puppeteer-element.ts | 22 |
src/runtime/client-hydrate.ts | 20 |
src/screenshot/connector-base.ts | 19 |
src/runtime/vdom/vdom-render.ts | 17 |
src/compiler/config/test/validate-paths.spec.ts | 16 |
src/dev-server/request-handler.ts | 15 |
src/compiler/prerender/prerender-optimize.ts | 14 |
src/compiler/sys/stencil-sys.ts | 14 |
src/compiler/transpile/transpile-module.ts | 14 |
src/sys/node/node-sys.ts | 14 |
src/compiler/prerender/prerender-queue.ts | 13 |
src/compiler/sys/in-memory-fs.ts | 13 |
src/runtime/connected-callback.ts | 13 |
src/runtime/set-value.ts | 13 |
src/compiler/output-targets/output-www.ts | 12 |
src/compiler/transformers/test/parse-vdom.spec.ts | 12 |
Our most common errors
Typescript Error Code | Count |
---|---|
TS2345 | 366 |
TS2322 | 362 |
TS18048 | 224 |
TS18047 | 99 |
TS2722 | 37 |
TS2532 | 26 |
TS2531 | 23 |
TS2454 | 14 |
TS2352 | 12 |
TS2790 | 11 |
TS2769 | 8 |
TS2538 | 8 |
TS2344 | 6 |
TS2416 | 6 |
TS2493 | 3 |
TS18046 | 2 |
TS2684 | 1 |
TS2430 | 1 |
Unused exports report
There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!
Unused exports
File | Line | Identifier |
---|---|---|
src/runtime/bootstrap-lazy.ts | 21 | setNonce |
src/screenshot/screenshot-fs.ts | 18 | readScreenshotData |
src/testing/testing-utils.ts | 198 | withSilentWarn |
src/utils/index.ts | 145 | CUSTOM |
src/utils/index.ts | 269 | normalize |
src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
src/compiler/app-core/app-data.ts | 25 | BUILD |
src/compiler/app-core/app-data.ts | 115 | Env |
src/compiler/app-core/app-data.ts | 117 | NAMESPACE |
src/compiler/fs-watch/fs-watch-rebuild.ts | 123 | updateCacheFromRebuild |
src/compiler/types/validate-primary-package-output-target.ts | 61 | satisfies |
src/compiler/types/validate-primary-package-output-target.ts | 61 | Record |
src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
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 can successfully reproduce the fix. The only nit I have is the any
cast of the window object. I am not sure how types are set up, if it requires to many changes we can keep as is.
👍
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.
LGTM! Found another related bug when I was testing this, but I think it's outside of the scope of what we're looking to do here. Captured details in STENCIL-1113
This fixes an issue where HMR updates which changed arguments to the
@Component
decorator would crash the application because aHostRef
for the component could not be located.This occurred because HMR updates caused a new version of the component's lazy bundle to be fetched, which in turn would bundle the
getHostRef
function and the correspondinghostRefs
variable, which is declared here:stencil/src/client/client-host-ref.ts
Line 18 in 0041dc2
Because the newly fetched lazy entry point for the updated component did not have access to the
hostRefs
variable which was in-scope in the old bundle, thegetHostRef
function would always returnundefined
on the first render after the HMR update, causing issues in functions likesetValue
which make use of aHostRef
.In order to fix the issue we can persist the
hostRefs
map across HMR updates by storing a reference to it on thewindow
object. We can additionally constrain this persistence to only occur when we're building with HMR, so that we don't clutter up thewindow
object of unsuspecting Stencil users in the context of a production build.What is the current behavior?
Unfortunately at present if you are using the hot-reload dev server and you make an edit to the
@Component
decorator of your component you'll see a big error in the browser console, and you'll be forced to reload 😢. The error will look something like this:Not such a nice time.
What is the new behavior?
Now if you make an edit to your
@Component
decorator you'll just see your component get updated in the browser with no errors.Does this introduce a breaking change?
Testing
In order to test this I would recommend the following:
npm start
) and edit a property in@Component
you see an error in the console like abovenpm start
) and confirm that editing the@Component
decorator doesn't cause catastrophic issues anymore