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

Prefetch CSS only once #4557

Closed
wants to merge 11 commits into from
Closed

Prefetch CSS only once #4557

wants to merge 11 commits into from

Conversation

Jelenkee
Copy link
Contributor

Changes

When multiple links where prefetched the same CSS files where fetched multiple times as well.
This PR changes the behaviour so that a CSS file is not fetched more than once.

Testing

Docs

@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2022

🦋 Changeset detected

Latest commit: c52979f

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

This PR includes changesets to release 1 package
Name Type
@astrojs/prefetch Minor

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: integration Related to any renderer integration (scope) label Aug 30, 2022
@natemoo-re
Copy link
Member

This seems like a totally reasonable change! I'll ping @tony-sull to take a look at this as well. Any idea why tests are failing?

@natemoo-re natemoo-re requested a review from tony-sull August 31, 2022 15:32
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

lgtm

@Jelenkee
Copy link
Contributor Author

Jelenkee commented Sep 4, 2022

Sorry, I am not able to run the tests on my local machine.
I use the ubuntu docker image to run the tests but I get some errors which are not related to my changes (I assume).

@astrojs/prefetch:test: Running 4 tests using 1 worker
@astrojs/prefetch:test: 
@astrojs/prefetch:test:   ✘  1 [Chrome Stable] › test/basic-prefetch.test.js:19:4 › Basic prefetch › dev › prefetches rel="prefetch" links › skips /admin (147ms)
@astrojs/prefetch:test: [vite:reporter] process.stdout.clearLine is not a function
@astrojs/prefetch:test:   ✘  2 [Chrome Stable] › test/basic-prefetch.test.js:55:4 › Basic prefetch › build › prefetches rel="prefetch" links › skips /admin (2ms)
@astrojs/prefetch:test:   ✘  3 [Chrome Stable] › test/custom-selectors.test.js:27:4 › Custom prefetch selectors › dev › prefetches links by custom selector › only prefetches /contact (155ms)
@astrojs/prefetch:test: [vite:reporter] process.stdout.clearLine is not a function
@astrojs/prefetch:test:   ✘  4 [Chrome Stable] › test/custom-selectors.test.js:60:4 › Custom prefetch selectors › build › prefetches links by custom selector › only prefetches /contact (2ms)
@astrojs/prefetch:test: 
@astrojs/prefetch:test: 
@astrojs/prefetch:test:   1) [Chrome Stable] › test/basic-prefetch.test.js:19:4 › Basic prefetch › dev › prefetches rel="prefetch" links › skips /admin 
@astrojs/prefetch:test: 
@astrojs/prefetch:test:     browserType.launch: Chromium distribution 'chrome' is not found at /opt/google/chrome/chrome
@astrojs/prefetch:test:     Run "npx playwright install chrome"
@astrojs/prefetch:test: 
@astrojs/prefetch:test:         at Object._baseTest.extend.browser.scope [as fn] (/astro/node_modules/.pnpm/@[email protected]/node_modules/@playwright/test/lib/index.js:222:51)
@astrojs/prefetch:test:         at /astro/node_modules/.pnpm/@[email protected]/node_modules/@playwright/test/lib/fixtures.js:112:81
@astrojs/prefetch:test: 
@astrojs/prefetch:test:   2) [Chrome Stable] › test/basic-prefetch.test.js:55:4 › Basic prefetch › build › prefetches rel="prefetch" links › skips /admin 
@astrojs/prefetch:test: 
@astrojs/prefetch:test:     TypeError: process.stdout.clearLine is not a function
@astrojs/prefetch:test: 
@astrojs/prefetch:test:       43 |
@astrojs/prefetch:test:       44 | 		test.beforeAll(async ({ astro }) => {
@astrojs/prefetch:test:     > 45 | 			await astro.build();
@astrojs/prefetch:test:          | 			^
@astrojs/prefetch:test:       46 | 			previewServer = await astro.preview();
@astrojs/prefetch:test:       47 | 		});
@astrojs/prefetch:test:       48 |
@astrojs/prefetch:test: 
@astrojs/prefetch:test:         at Object.buildEnd (file:///astro/node_modules/.pnpm/[email protected][email protected]/node_modules/vite/dist/node/chunks/dep-0fc8e132.js:12619:36)
@astrojs/prefetch:test:         at file:///astro/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/rollup.js:22695:37
@astrojs/prefetch:test:         at async Promise.all (index 3)
@astrojs/prefetch:test:         at file:///astro/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/rollup.js:23649:13
@astrojs/prefetch:test:         at catchUnfinishedHookActions (file:///astro/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/rollup.js:23041:20)
@astrojs/prefetch:test:         at rollupInternal (file:///astro/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/rollup.js:23639:5)
@astrojs/prefetch:test:         at doBuild (file:///astro/node_modules/.pnpm/[email protected][email protected]/node_modules/vite/dist/node/chunks/dep-0fc8e132.js:43620:24)
@astrojs/prefetch:test:         at Module.build (file:///astro/node_modules/.pnpm/[email protected][email protected]/node_modules/vite/dist/node/chunks/dep-0fc8e132.js:43468:16)
@astrojs/prefetch:test:         at clientBuild (file:///astro/packages/astro/dist/core/build/static-build.js:195:23)
@astrojs/prefetch:test:         at staticBuild (file:///astro/packages/astro/dist/core/build/static-build.js:64:3)
@astrojs/prefetch:test:         at AstroBuilder.build (file:///astro/packages/astro/dist/core/build/index.js:83:5)
@astrojs/prefetch:test:         at AstroBuilder.run (file:///astro/packages/astro/dist/core/build/index.js:124:7)
@astrojs/prefetch:test:         at build (file:///astro/packages/astro/dist/core/build/index.js:22:3)
@astrojs/prefetch:test:         at file:///astro/packages/integrations/prefetch/test/basic-prefetch.test.js:45:4
@astrojs/prefetch:test: 
@astrojs/prefetch:test:   3) [Chrome Stable] › test/custom-selectors.test.js:27:4 › Custom prefetch selectors › dev › prefetches links by custom selector › only prefetches /contact 
@astrojs/prefetch:test: 
@astrojs/prefetch:test:     browserType.launch: Chromium distribution 'chrome' is not found at /opt/google/chrome/chrome
@astrojs/prefetch:test:     Run "npx playwright install chrome"
@astrojs/prefetch:test: 
@astrojs/prefetch:test:         at Object._baseTest.extend.browser.scope [as fn] (/astro/node_modules/.pnpm/@[email protected]/node_modules/@playwright/test/lib/index.js:222:51)
@astrojs/prefetch:test:         at /astro/node_modules/.pnpm/@[email protected]/node_modules/@playwright/test/lib/fixtures.js:112:81
@astrojs/prefetch:test: 
@astrojs/prefetch:test:   4) [Chrome Stable] › test/custom-selectors.test.js:60:4 › Custom prefetch selectors › build › prefetches links by custom selector › only prefetches /contact 
@astrojs/prefetch:test: 
@astrojs/prefetch:test:     TypeError: process.stdout.clearLine is not a function
@astrojs/prefetch:test: 
@astrojs/prefetch:test:       48 |
@astrojs/prefetch:test:       49 | 		test.beforeAll(async ({ astro }) => {
@astrojs/prefetch:test:     > 50 | 			await astro.build();
@astrojs/prefetch:test:          | 			^
@astrojs/prefetch:test:       51 | 			previewServer = await astro.preview();
@astrojs/prefetch:test:       52 | 		});
@astrojs/prefetch:test:       53 |
@astrojs/prefetch:test: 
@astrojs/prefetch:test:         at Object.buildEnd (file:///astro/node_modules/.pnpm/[email protected][email protected]/node_modules/vite/dist/node/chunks/dep-0fc8e132.js:12619:36)
@astrojs/prefetch:test:         at file:///astro/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/rollup.js:22695:37
@astrojs/prefetch:test:         at async Promise.all (index 3)
@astrojs/prefetch:test:         at file:///astro/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/rollup.js:23649:13
@astrojs/prefetch:test:         at catchUnfinishedHookActions (file:///astro/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/rollup.js:23041:20)
@astrojs/prefetch:test:         at rollupInternal (file:///astro/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/rollup.js:23639:5)
@astrojs/prefetch:test:         at doBuild (file:///astro/node_modules/.pnpm/[email protected][email protected]/node_modules/vite/dist/node/chunks/dep-0fc8e132.js:43620:24)
@astrojs/prefetch:test:         at Module.build (file:///astro/node_modules/.pnpm/[email protected][email protected]/node_modules/vite/dist/node/chunks/dep-0fc8e132.js:43468:16)
@astrojs/prefetch:test:         at clientBuild (file:///astro/packages/astro/dist/core/build/static-build.js:195:23)
@astrojs/prefetch:test:         at staticBuild (file:///astro/packages/astro/dist/core/build/static-build.js:64:3)
@astrojs/prefetch:test:         at AstroBuilder.build (file:///astro/packages/astro/dist/core/build/index.js:83:5)
@astrojs/prefetch:test:         at AstroBuilder.run (file:///astro/packages/astro/dist/core/build/index.js:124:7)
@astrojs/prefetch:test:         at build (file:///astro/packages/astro/dist/core/build/index.js:22:3)
@astrojs/prefetch:test:         at file:///astro/packages/integrations/prefetch/test/custom-selectors.test.js:50:4
@astrojs/prefetch:test: 
@astrojs/prefetch:test: 
@astrojs/prefetch:test:   4 failed
@astrojs/prefetch:test:     [Chrome Stable] › test/basic-prefetch.test.js:19:4 › Basic prefetch › dev › prefetches rel="prefetch" links › skips /admin 
@astrojs/prefetch:test:     [Chrome Stable] › test/basic-prefetch.test.js:55:4 › Basic prefetch › build › prefetches rel="prefetch" links › skips /admin 
@astrojs/prefetch:test:     [Chrome Stable] › test/custom-selectors.test.js:27:4 › Custom prefetch selectors › dev › prefetches links by custom selector › only prefetches /contact 
@astrojs/prefetch:test:     [Chrome Stable] › test/custom-selectors.test.js:60:4 › Custom prefetch selectors › build › prefetches links by custom selector › only prefetches /contact 
@astrojs/prefetch:test:  ELIFECYCLE  Test failed. See above for more details.
@astrojs/prefetch:test: Error: command finished with error: command (packages/integrations/prefetch) pnpm run test exited (1)

It's hard to write proper tests when you can only run them in Github Actions

.filter((el) => !loadedStyles.has(el.href))
.map((el) => {
loadedStyles.add(el.href);
return fetch(el.href);
Copy link
Member

Choose a reason for hiding this comment

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

This promise needed to be returned, which might fix the tests! Watching CI to see.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jelenkee can you try this?

@matthewp
Copy link
Contributor

Closing because the tests are failing. @Jelenkee If you get time, the above suggestion might fix the tests, in which case you can resubmit a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants