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(dev): only inject css link tags that have js importers #7267

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

thejackshelton
Copy link
Member

@thejackshelton thejackshelton commented Jan 18, 2025

Builds on top of #7266, otherwise I can't build Qwik.

Fixes #5965 in the Qwik vite dev server.

@thejackshelton thejackshelton requested review from a team as code owners January 18, 2025 21:05
Copy link

changeset-bot bot commented Jan 18, 2025

⚠️ No Changeset found

Latest commit: 2acab11

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

@thejackshelton thejackshelton changed the title Css dev imports Dev Server should only inject link tags that have js importers Jan 18, 2025
Copy link

pkg-pr-new bot commented Jan 18, 2025

Open in Stackblitz

npm i https://pkg.pr.new/@builder.io/qwik@7267
npm i https://pkg.pr.new/@builder.io/qwik-city@7267
npm i https://pkg.pr.new/eslint-plugin-qwik@7267
npm i https://pkg.pr.new/create-qwik@7267

commit: 2acab11

Copy link
Contributor

github-actions bot commented Jan 18, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
qwik-docs ✅ Ready (View Log) Visit Preview 2acab11

Varixo
Varixo previously approved these changes Jan 21, 2025
Copy link
Member

@Varixo Varixo left a comment

Choose a reason for hiding this comment

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

Awesome!

Copy link
Member

Choose a reason for hiding this comment

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

Please revert this file :D

Copy link
Member Author

Choose a reason for hiding this comment

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

it just adds it back every build

const allModules = Array.from(server.moduleGraph.fileToModulesMap.entries());

const CSS_EXTENSIONS = ['.css', '.scss', '.sass', '.less', '.styl', '.stylus'];
const JS_EXTENSIONS = ['.js', '.ts', '.tsx', '.jsx'];
Copy link
Member

Choose a reason for hiding this comment

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

/.([mc]?[tj]sx?$/.test(ext)/

Copy link
Member

Choose a reason for hiding this comment

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

there is also .mjs and everything with m

Copy link
Member

@PatrickJS PatrickJS Jan 21, 2025

Choose a reason for hiding this comment

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

also we need to escape the .
/\.

Copy link
Member Author

Choose a reason for hiding this comment

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

both added as regexes now

const added = new Set();
const allModules = Array.from(server.moduleGraph.fileToModulesMap.entries());

const CSS_EXTENSIONS = ['.css', '.scss', '.sass', '.less', '.styl', '.stylus'];
Copy link
Member

@PatrickJS PatrickJS Jan 21, 2025

Choose a reason for hiding this comment

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

we should let the dev add or configure this just in-case the extensions change. but these are good defaults of course

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd need to modify the vite plugin API to do that. In which case we already plan to move the dev server to Qwik Router, so we should wait in this case before any API changes.

@@ -3653,7 +3653,7 @@ Options for the prefetch service worker.
</tbody></table>
**Returns:**
[JSXNode](#jsxnode)&lt;'script'&gt;
JSXNode&lt;'script'&gt;
Copy link
Member

Choose a reason for hiding this comment

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

Please run pnpm api.update. Unfortunately the build messes with this.

Copy link
Member

Choose a reason for hiding this comment

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

please undo

@@ -192,26 +199,11 @@ export async function configureDevServer(

const result = await render(renderOpts);

// Sometimes new CSS files are added after the initial render
Array.from(server.moduleGraph.fileToModulesMap.entries()).forEach((entry) => {
Copy link
Member

Choose a reason for hiding this comment

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

Aren't you now ignoring newly added css modules?

I think it would be better to generate the current list of css files on every update and make sure only those are injected?

@wmertens wmertens changed the title Dev Server should only inject link tags that have js importers fix(dev): only inject css link tags that have js importers Jan 22, 2025
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.

[🐞] Error with TailwindCSS 4 when used in Qwik
4 participants