Fix Windows dev script proxying #2052

drwpow · 2021-11-29T23:08:01Z

Changes

Fixes #2053. In dev, Windows was getting bad URLs for script proxying. This behavior only existed in dev, not build, so dev tests needed to be added that we didn’t have before.

Testing

Tests added that didn’t exist before.

Docs

No docs changes.

changeset-bot · 2021-11-29T23:08:06Z

⚠️ No Changeset found

Latest commit: 2ee36e0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

netlify · 2021-11-29T23:08:07Z

✔️ Deploy Preview for astro-www ready!

🔨 Explore the source changes: 2ee36e0

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-www/deploys/61a577acaf48e10007b6ce74

😎 Browse the preview: https://deploy-preview-2052--astro-www.netlify.app

netlify · 2021-11-29T23:08:07Z

✔️ Deploy Preview for astro-docs-2 ready!

🔨 Explore the source changes: 2ee36e0

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-docs-2/deploys/61a577ace559090007d92ba3

😎 Browse the preview: https://deploy-preview-2052--astro-docs-2.netlify.app

#2053

drwpow · 2021-11-30T16:37:57Z

packages/astro/src/core/ssr/index.ts

@@ -266,7 +266,8 @@ export async function render(renderers: Renderer[], mod: ComponentInstance, ssrO

 // run transformIndexHtml() in dev to run Vite dev transformations
 if (mode === 'development') {
- html = await viteServer.transformIndexHtml(viteifyURL(filePath), html, pathname);
+ const relativeURL = filePath.href.replace(astroConfig.projectRoot.href, '/');
+ html = await viteServer.transformIndexHtml(relativeURL, html, pathname);


Big change 1: use project-relative URLs to generate the script proxies. Here’s the difference:

- <script type="module" src="/:/Users/astro/code/my-project/src/pages/index.astro?html&proxy=0.js"> + <script type="module" src="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/index.astro?html&proxy=0.js">

Note the /:/ is because C:/ isn’t the start of a browser-compatible pathname

drwpow · 2021-11-30T16:42:07Z

packages/astro/src/core/ssr/css.ts

 // mark the entrypoint as scanned to avoid an infinite loop
- scanned.add(moduleName.id);
+ scanned.add(moduleName.url);


Big change 2: use url instead of id in Vite’s module graph. Here’s the difference between the two, according to Vite:

ID

Linux/Mac: /Users/astro/code/my-project/src/pages/index.astro ✅ valid in module graph & in browser

Windows: C:/Users/astro/code/my-project/src/pages/index.astro ❌ valid in module graph but not valid in browser (the original bug)

URL

Linux/Mac: /@fs/Users/astro/code/my-project/src/pages/index.astro ✅ valid in module graph & in browser

Windows: /@fs/C:/Users/astro/code/my-project/src/pages/index.astro ✅ valid in module graph & in browser

This PR uses url so that the same value can be placed in the browser and resolved.

drwpow · 2021-11-30T16:42:30Z

packages/astro/src/core/util.ts

 */
 export function viteifyURL(filePath: URL): string {
- return slash(fileURLToPath(filePath));
+ return `/@fs${filePath.pathname}`;


Updates the viteifyURL function to use module graph url over id

drwpow · 2021-11-30T16:44:54Z

packages/astro/test/react-component.test.js

- if (match[1].includes('Research.js')) {
- componentUrl = match[1];
- break;
+ describe('dev', () => {


No build tests changed; only added “dev” tests

drwpow · 2021-11-30T16:45:38Z

packages/astro/src/core/dev/index.ts

- } else {
- this.viteServer.moduleGraph.invalidateAll();
- }
+ await this.viteServer.moduleGraph.invalidateAll();


Last change: when something threw an error, we were doing too much work to only invalidate some modules, possibly missing many in the process. If something has erred, just invalidate everything 🤷🏻.

tony-sull

Nice! These fixes to handle Windows filenames and the test updates look great to me 👍

Added one note on a skipped test, but I'm not 100% sure on whether the test is useful or not and don't think that should block merging in these fixes

tony-sull · 2021-11-30T17:51:39Z

packages/astro/test/react-component.test.js

- https://docs.astro.build/reference/api-reference/#importmeta`
- );
- });
+ // TODO: is this still a relevant test?


Hmm, it seems like this should still be a useful test?

Test #2 checks that data-reactroot isn't added to the test page's <Hello /> component since it isn't using client hydration. It looks like this test would be making sure that data-reactroot is still used on the hydrated <Research /> component

What I'm not actually sure on is whether data-reactroot is still used to flag hydrated react components. If yes, I think this and test #2 should be useful, if no I think both tests aren't helpful anymore (since test #2 would always get undefined regardless of how hydration was used)

So this test has been skipped since the move to Vite/0.21. @natemoo-re what should this test look for now?

I believe data-reactroot is required for hydrated components still. If this test is broken it might be a symptom of a bug we need to fix. Happy to spin off into a separate PR.

Sounds good. Yeah if that could be investigated separately (ideally testing dev and build) that would be good. This test needs to be updated, but I wasn’t sure exactly how (and is unrelated to this PR).

* Add tests for script proxying * Fix Windows script proxying withastro#2053

github-actions bot added pkg: astro Related to the core `astro` package (scope) test labels Nov 29, 2021

drwpow force-pushed the script-proxy-tests branch from 94bf944 to cbe484f Compare November 29, 2021 23:14

Add tests for script proxying

d39879b

drwpow force-pushed the script-proxy-tests branch 2 times, most recently from 6c9e964 to 3f717b4 Compare November 30, 2021 00:59

drwpow changed the title ~~[wip] Add tests for script proxying~~ [wip] Fix Windows dev script proxying Nov 30, 2021

Fix Windows script proxying

2ee36e0

#2053

drwpow force-pushed the script-proxy-tests branch from 3f717b4 to 2ee36e0 Compare November 30, 2021 01:00

drwpow changed the title ~~[wip] Fix Windows dev script proxying~~ Fix Windows dev script proxying Nov 30, 2021

drwpow commented Nov 30, 2021

View reviewed changes

drwpow marked this pull request as ready for review November 30, 2021 16:45

tony-sull approved these changes Nov 30, 2021

View reviewed changes

drwpow merged commit 03cabc5 into main Nov 30, 2021

drwpow deleted the script-proxy-tests branch November 30, 2021 18:54

drwpow mentioned this pull request Nov 30, 2021

Makes Astro.resolve return root-relative paths #2048

Merged

drwpow added a commit that referenced this pull request Dec 1, 2021

Add changeset for #2052

5c6fae7

drwpow mentioned this pull request Dec 1, 2021

Add changeset for #2052 #2066

Merged

drwpow added a commit that referenced this pull request Dec 1, 2021

Add changeset for #2052 (#2066)

f5efbe1

drwpow added a commit that referenced this pull request Dec 1, 2021

Modify changeset for #2052

a9bcfc6

drwpow added a commit that referenced this pull request Dec 1, 2021

Modify changeset for #2052 (#2067)

96ba37e

IanVS mentioned this pull request Dec 3, 2021

🐛 BUG: Several pages of Astro docs are missing header styles #2099

Closed

SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023

Fix Windows dev script proxying (withastro#2052)

69dc097

* Add tests for script proxying * Fix Windows script proxying withastro#2053

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Windows dev script proxying #2052

Fix Windows dev script proxying #2052

drwpow commented Nov 29, 2021 •

edited

Loading

changeset-bot bot commented Nov 29, 2021 •

edited

Loading

netlify bot commented Nov 29, 2021 •

edited

Loading

netlify bot commented Nov 29, 2021 •

edited

Loading

drwpow Nov 30, 2021 •

edited

Loading

drwpow Nov 30, 2021

drwpow Nov 30, 2021 •

edited

Loading

drwpow Nov 30, 2021

drwpow Nov 30, 2021

drwpow Nov 30, 2021 •

edited

Loading

tony-sull left a comment

tony-sull Nov 30, 2021

drwpow Nov 30, 2021

natemoo-re Nov 30, 2021

drwpow Nov 30, 2021

Fix Windows dev script proxying #2052

Fix Windows dev script proxying #2052

Conversation

drwpow commented Nov 29, 2021 • edited Loading

Changes

Testing

Docs

changeset-bot bot commented Nov 29, 2021 • edited Loading

⚠️ No Changeset found

netlify bot commented Nov 29, 2021 • edited Loading

netlify bot commented Nov 29, 2021 • edited Loading

drwpow Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

drwpow Nov 30, 2021

Choose a reason for hiding this comment

drwpow Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

ID

URL

drwpow Nov 30, 2021

Choose a reason for hiding this comment

drwpow Nov 30, 2021

Choose a reason for hiding this comment

drwpow Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

tony-sull left a comment

Choose a reason for hiding this comment

tony-sull Nov 30, 2021

Choose a reason for hiding this comment

drwpow Nov 30, 2021

Choose a reason for hiding this comment

natemoo-re Nov 30, 2021

Choose a reason for hiding this comment

drwpow Nov 30, 2021

Choose a reason for hiding this comment

drwpow commented Nov 29, 2021 •

edited

Loading

changeset-bot bot commented Nov 29, 2021 •

edited

Loading

netlify bot commented Nov 29, 2021 •

edited

Loading

netlify bot commented Nov 29, 2021 •

edited

Loading

drwpow Nov 30, 2021 •

edited

Loading

drwpow Nov 30, 2021 •

edited

Loading

drwpow Nov 30, 2021 •

edited

Loading