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(optimizer): transform code incrementally #6326

Closed
wants to merge 2 commits into from

Conversation

wmertens
Copy link
Member

@wmertens wmertens commented May 16, 2024

This builds on top of #6539 (which was first part of this).

Qwik should not split files before other build plugins had a chance to change the code.

The problem is that Qwik outputs "entry files" that contain multiple QRL segments, possibly from several different files, and that Rollup doesn't allow changing a file once it was loaded.

So this PR will need to enforce ordering. Not done yet.

Copy link

netlify bot commented May 16, 2024

Deploy Preview for qwik-insights failed.

Name Link
🔨 Latest commit e6e4277
🔍 Latest deploy log https://app.netlify.com/sites/qwik-insights/deploys/666bf6bb7d0e5a00085d737f

Copy link

cloudflare-workers-and-pages bot commented May 16, 2024

Deploying qwik-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: e6e4277
Status:🚫  Build failed.

View logs

@wmertens wmertens force-pushed the hooks-relative-paths branch 2 times, most recently from e9bda50 to c412f9c Compare May 16, 2024 13:02
@shairez
Copy link
Contributor

shairez commented May 17, 2024

This fixes this issue right?
#5473

@wmertens wmertens force-pushed the hooks-relative-paths branch 3 times, most recently from 4a2bd34 to c3f2444 Compare May 20, 2024 16:13
@wmertens wmertens enabled auto-merge (rebase) May 20, 2024 16:28
@wmertens wmertens disabled auto-merge May 20, 2024 22:10
@wmertens wmertens force-pushed the hooks-relative-paths branch 5 times, most recently from 60a0b16 to a8d3e57 Compare May 24, 2024 21:00
@wmertens wmertens requested a review from mhevery May 24, 2024 21:04
@wmertens wmertens force-pushed the hooks-relative-paths branch from a8d3e57 to 546e7f9 Compare May 27, 2024 13:47
@wmertens wmertens requested review from a team as code owners May 27, 2024 13:47
@wmertens wmertens force-pushed the hooks-relative-paths branch from 546e7f9 to 656c8b2 Compare May 27, 2024 21:05
@wmertens
Copy link
Member Author

grr the small stuff is also going nowhere. Needs to be the big thing. Work in progress. I need to get dev mode working and then it looks like it will be very very nice

@wmertens wmertens marked this pull request as draft May 28, 2024 14:18
@wmertens wmertens force-pushed the hooks-relative-paths branch 5 times, most recently from e905d33 to bce7eed Compare May 31, 2024 17:39
@@ -102,10 +102,12 @@ export const qwikLoader = (
for (const qrl of attrValue.split('\n')) {
const url = new URL(qrl, base);
const href = url.href;
const symbol = url.hash[replace](/^#?([^?[|]*).*$/, '$1') || 'default';
const match = /^#?(([^#]+)#)?([^?[|]*).*$/.exec(url.hash);
Copy link
Member

Choose a reason for hiding this comment

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

can we add regexp online editor link. for example https://regexr.com/ lets you also add regexp tests for example https://regexr.com/816es (don't use mine we should make a qwik account with all them saved

@PatrickJS
Copy link
Member

this pr #6017 will effect this one

@wmertens
Copy link
Member Author

wmertens commented Jun 7, 2024

@PatrickJS actually I think it supercedes it - I think the problem in dev mode is chunk sources from unknown parents and this solves that.

Although a quick fix would be to add the prefix in the existing vite-server.ts

@wmertens wmertens force-pushed the hooks-relative-paths branch 2 times, most recently from 2f0f17a to 994d30d Compare June 12, 2024 08:56
Copy link

pkg-pr-new bot commented Jun 12, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

commit: e6e4277

@builder.io/qwik

npm i https://pkg.pr.new/@builder.io/qwik@6326

@builder.io/qwik-city

npm i https://pkg.pr.new/@builder.io/qwik-city@6326

eslint-plugin-qwik

npm i https://pkg.pr.new/eslint-plugin-qwik@6326

create-qwik

npm i https://pkg.pr.new/create-qwik@6326

@wmertens wmertens force-pushed the hooks-relative-paths branch 2 times, most recently from 6b09472 to d678a5c Compare June 12, 2024 20:52
wmertens added 2 commits June 14, 2024 09:52
+ remove import-of-import workaround

Transforming breaks other vite plugins, slows down dev start and might transform more than needed
Now the bundler can decide which QRLs to host toghether.

- chunk URLs now can include the exported attr as `chunk#attr`
- manifest generation is now more robust
- refactored manifest generation
@wmertens wmertens force-pushed the hooks-relative-paths branch from d678a5c to e6e4277 Compare June 14, 2024 07:52
@wmertens wmertens changed the title fix(optimizer): retain relative paths in extracted code fix(optimizer): transform code incrementally Jun 14, 2024
@gioboa gioboa linked an issue Jun 14, 2024 that may be closed by this pull request
@wmertens
Copy link
Member Author

closing in favor of #6670

@wmertens wmertens closed this Jul 28, 2024
@shairez shairez deleted the hooks-relative-paths branch October 30, 2024 16:01
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.

[🐞] weirdness with CSS import names in dev
3 participants