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

feat: rollup 3 #9870

Merged
merged 13 commits into from Nov 7, 2022
Merged

feat: rollup 3 #9870

merged 13 commits into from Nov 7, 2022

Conversation

patak-dev
Copy link
Member

Description

Updates Vite to Rollup 3. The main change is a new hashing algorithm:

Check out the description of that PR to understand the changes. There was an issue raised in rollup because the characters used as placeholders for the hashes of chunk file names are being encoded by Vite, preventing Rollup to replace them. See rollup/rollup#4618 (comment)

The problem was that esbuild by default runs in ascii only mode. You can see an example in this playground

We can solve this issue by using charset: 'utf-8' for esbuild vitejs/vite@feat/rollup-3?expand=1#diff-6d149ac970

I think that independently of Rollup 3, we should be using utf-8 for the encoding as this is the default for HTML5 anyways.

WIP, some tests are expected to fail. Pushing the branch now so we can discuss about this issue and check if the scheme used by Rollup with these non-ascii placeholders will work ok with Vite.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev added this to the 4.0 milestone Aug 26, 2022
@bluwy bluwy added the p3-significant High priority enhancement (priority) label Aug 28, 2022
@balu-lt
Copy link

balu-lt commented Oct 11, 2022

Rollup 3.0 stable was released! 🥳

@bluwy
Copy link
Member

bluwy commented Nov 2, 2022

Pushed a commit to handle pure CSS chunks mapping between renderChunk and generateBundle, as the !~{XXX}~ placeholder in the chunk.fileName interferes with comparisons.

I also noticed a strange error when running pnpm dev in packages/vite and pnpm test in the root. Getting this from Vitest:

Error: Parse failure: Unexpected token (52:24)
Contents of line 52:            this.#head =          ;

Can't find the source, but it's in Vitest's chunk-integrations-coverage.cca09977.js file that's having issues. pnpm build then pnpm test works fine.


I also haven't fully reviewed the assertions addition, but I assume we want that to be optional by default?

@patak-dev
Copy link
Member Author

Pushed a commit to handle pure CSS chunks mapping between renderChunk and generateBundle, as the !~{XXX}~ placeholder in the chunk.fileName interferes with comparisons.

Awesome! And this made it all green!

I also noticed a strange error when running pnpm dev in packages/vite and pnpm test in the root. Getting this from Vitest:

Error: Parse failure: Unexpected token (52:24)
Contents of line 52:            this.#head =          ;

Can't find the source, but it's in Vitest's chunk-integrations-coverage.cca09977.js file that's having issues. pnpm build then pnpm test works fine.

Where are we using private properties? 🤔

I also haven't fully reviewed the assertions addition, but I assume we want that to be optional by default?

What we did so far is pass empty objects (meaning no type assertions) to resolveId et al options because they now require it. I think we should emulate what rollup does and forward the assertion so plugins can decide what to do with it. What do you mean by making it optional by default?

@sapphi-red
Copy link
Member

Can't find the source, but it's in Vitest's chunk-integrations-coverage.cca09977.js file that's having issues. pnpm build then pnpm test works fine.

Maybe #10740 will fix this. It sounds similar.

@bluwy
Copy link
Member

bluwy commented Nov 4, 2022

Ah yes that should fix it. The raw code had this.#head = undefined; so that explains the error message.

@bluwy
Copy link
Member

bluwy commented Nov 6, 2022

What we did so far is pass empty objects (meaning no type assertions) to resolveId et al options because they now require it. I think we should emulate what rollup does and forward the assertion so plugins can decide what to do with it. What do you mean by making it optional by default?

Yeah I noticed that Rollup now supports assertions too, but I think the API should have the property optional, e.g. you don't need to pluginContainer.resolvedId(id, importer, { assertions: {} }) everywhere. assertions should fallback to {} instead if undefined by itself, so you'd only have to do pluginContainer.resolvedId(id, importer)

@bluwy
Copy link
Member

bluwy commented Nov 6, 2022

pnpm dev and pnpm test works for me now after merging main. I also made the assertions optional following my comment above, but happy to revert that if I mistaken the change.

@patak-dev
Copy link
Member Author

I also made the assertions optional following my comment above, but happy to revert that if I mistaken the change.

Looks good! It seems I checked one of the internal rollup definitions where Lukas is forcing the code to pass empty assertions to be explicit. Your change should be good on our side.
Let's merge this so tomorrow we can start the alpha.

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Nov 6, 2022

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ❌ failure
iles ❌ failure
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ❌ failure
rakkas ✅ success
svelte ❌ failure
vite-plugin-ssr ❌ failure
vite-setup-catalogue ✅ success
vitepress ❌ failure
vitest ✅ success
windicss ✅ success

@patak-dev patak-dev merged commit beb7166 into main Nov 7, 2022
@patak-dev patak-dev deleted the feat/rollup-3 branch November 7, 2022 08:35
@jacekkarczmarczyk
Copy link

Thanks for this PR and early release of pre 4.0!

@patak-dev
Copy link
Member Author

Any feedback if you test v4-alpha.0 is appreciated @jacekkarczmarczyk! Our idea is to work with the ecosystem to fix the issues vite-ecosystem-ci uncovered above and ask for wider testing at a later stage (probably for beta.1 as usual). We are going to try to keep Vite 4 smaller than v3, so if things goes well, we should be able to release it mid-december

@jacekkarczmarczyk
Copy link

Sure, I'm going to try it out today on the code from my issue here as well as on my actual project (which is yet to be converted to vite, because of that problem)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants