Skip to content
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: isSelfAccepting? More like isBanishedToTheShadowRealm #2852

Merged
merged 5 commits into from
Mar 22, 2022

Conversation

bholmesdev
Copy link
Contributor

Changes

Testing

Had to test by hand, manually copying the dist to a node_modules folder to avoid false positives. I encourage other devs to do the same, or install directly from this GitHub branch!

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Mar 21, 2022

🦋 Changeset detected

Latest commit: 15dbbf7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
astro Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Mar 21, 2022
@@ -75,7 +83,7 @@ export async function render(renderers: SSRLoadedRenderer[], mod: ComponentInsta
children: '',
});
scripts.add({
props: { type: 'module', src: '/@id/astro/client/hmr.js' },
props: { type: 'module', src: new URL('../../../runtime/client/hmr.js', import.meta.url).pathname },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is surprising! Any idea why this was required / what kind of error message you were seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really is! I was seeing the following when Vite tried to resolve the module. Notice Astro returns a 404 as well:

14:23 PM [vite] Internal server error: Cannot set property 'isSelfAccepting' of undefined
  Plugin: vite:import-analysis
  File: /Users/benholmes/Sandbox/astro-integration-react/node_modules/.vite/astro_client_load_js.js?v=7a24946c
      at ModuleGraph.updateModuleInfo (/Users/benholmes/Sandbox/astro-integration-react/node_modules/vite/dist/node/chunks/dep-9c153816.js:53418:29)
      at TransformContext.transform (/Users/benholmes/Sandbox/astro-integration-react/node_modules/vite/dist/node/chunks/dep-9c153816.js:70072:57)
      at async Object.transform (/Users/benholmes/Sandbox/astro-integration-react/node_modules/vite/dist/node/chunks/dep-9c153816.js:38334:30)
      at async doTransform (/Users/benholmes/Sandbox/astro-integration-react/node_modules/vite/dist/node/chunks/dep-9c153816.js:53030:29)
7:14:23 PM [vite] Internal server error: Cannot set property 'isSelfAccepting' of undefined
  Plugin: vite:import-analysis
  File: /Users/benholmes/Sandbox/astro-integration-react/node_modules/.vite/astro_client_hmr_js.js?v=7a24946c
      at ModuleGraph.updateModuleInfo (/Users/benholmes/Sandbox/astro-integration-react/node_modules/vite/dist/node/chunks/dep-9c153816.js:53418:29)
      at TransformContext.transform (/Users/benholmes/Sandbox/astro-integration-react/node_modules/vite/dist/node/chunks/dep-9c153816.js:70072:57)
      at async Object.transform (/Users/benholmes/Sandbox/astro-integration-react/node_modules/vite/dist/node/chunks/dep-9c153816.js:38334:30)
      at async doTransform (/Users/benholmes/Sandbox/astro-integration-react/node_modules/vite/dist/node/chunks/dep-9c153816.js:53030:29)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ After tracing Vite's internal logs, there's a different ?v=XXXXX attached to the end of the URL between calls. It seems to think the file changed even when it hasn't...

@@ -40,11 +40,19 @@ export type ComponentPreload = [SSRLoadedRenderer[], ComponentInstance];
export type RenderResponse = { type: 'html'; html: string } | { type: 'response'; response: Response };

const svelteStylesRE = /svelte\?svelte&type=style/;
const rendererCache = new Map<string, SSRLoadedRenderer['ssr']>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you leave a comment here that this is required to prevent bugs? I removed it assuming it was no longer needed for perf, but had no idea it was running defense on bugs!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm curious if it ever was running defense on bugs to be honest. It seemed like a performance choice to me as well. I'll add a note 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

@FredKSchott
Copy link
Member

Also... changeset :)

@bholmesdev
Copy link
Contributor Author

Also... changeset :)

I'll never change(set) this is who I am

@FredKSchott
Copy link
Member

LGTM! Merging for you so that we can get a release out to test against

@FredKSchott FredKSchott merged commit 96372e6 into main Mar 22, 2022
@FredKSchott FredKSchott deleted the fix/i-do-not-accept-is-self-accepting branch March 22, 2022 03:14
This was referenced Mar 22, 2022
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
…stro#2852)

* fix: restore renderer caching strategy

* fix: restore old URL constructor for HMR

* docs: comment why we need the rendererCache

* refactor: remove needless "else"

* chore: changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants