Skip to content

fix: keep Build.isBrowser branches removable in hydrate builds - #761

Open
Unipro-DanMunn wants to merge 1 commit into
stenciljs:mainfrom
Unipro-DanMunn:fix/hydrate-tree-shaking
Open

Unipro-DanMunn wants to merge 1 commit into
stenciljs:mainfrom
Unipro-DanMunn:fix/hydrate-tree-shaking

Conversation

@Unipro-DanMunn

Copy link
Copy Markdown

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Tests (npm test) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Since 2.2.2, src/subscriptions/stencil.ts copies the @stencil/core namespace into a variable (const core = StencilCore). Rollup then keeps the whole module object, so it can no longer fold Build.isBrowser to false in Stencil's hydrate build. Any browser-only module imported behind if (Build.isBrowser) is bundled into the hydrate app and runs when it loads, so renderToString can throw (for example document.createTreeWalker is not a function).

GitHub Issue Number: #760

What is the new behavior?

  • forceUpdate and getRenderingRef are read straight off the namespace (StencilCore.forceUpdate), which Rollup resolves to the individual exports, so Build.isBrowser branches are removed from hydrate builds again.
  • The namespace import from chore(tests): Upgrade vitest fixing the issues and increment test coverage #647 stays, so a @stencil/core without these exports still gets an empty subscription instead of a missing-export error.
  • New test test/hydrate.test.ts: it bundles the store as it's published, installs it into a small Stencil app (test/hydrate-app) with a dist-hydrate-script output, builds that app, and checks that a module imported behind Build.isBrowser isn't in the hydrate app.

Does this introduce a breaking change?

  • Yes
  • No

Testing

Other information

The test installs the store into the fixture's node_modules instead of importing it from src, because the bug only reproduces when Stencil consumes the store as pre-built JavaScript. The fixture has its own package.json so TypeScript doesn't resolve @stencil/store to this repo's source through package self-reference.

Since 2.2.2 the Stencil subscription copied the `@stencil/core` namespace into a
variable. That makes Rollup keep the whole module object, so it can no longer
fold `Build.isBrowser` to `false` in Stencil's hydrate build, and browser-only
modules imported behind that check get bundled into the hydrate app.

Read `forceUpdate` and `getRenderingRef` straight off the namespace instead, which
keeps tolerating a `@stencil/core` without those exports. Adds a test that builds
a small Stencil app with a `dist-hydrate-script` output and checks the output.
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.

1 participant