-
Notifications
You must be signed in to change notification settings - Fork 188
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
[Vite Plugin] Fix vite manifest location #841
Conversation
…nifest is in .vite/manifest.json. Fixing bug that prevented building
🦋 Changeset detectedLatest commit: cf0758b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
any plans on merging? |
Would be nice to have any feedback on this... |
this will allow upgrading to Vite@5, am I correct? |
Honestly, I have no idea... |
@kaminskypavel Just noticed something, I am already using Vite@5... |
@jacksteamdev @AmySteam what do you guys think? |
@ale-ben I'm still getting the same following error, while using The upgrading to 5.0.x is not working. |
@nandanito this is the project I'm testing with. Just to be on the safe side, I rebuilt the vite plugin from my main branch and manually put it in my project. This is the build outputs: Can you provide an example of when it doesn't work? |
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ale-ben Thanks for the PR! Awesome research and deduction to get this working with Vite 5! 🌟
It looks like this is a breaking change for Vite 4, but maybe that's not a big deal? We could release this in one of two ways:
- Major version bump and only support Vite 5... This will require updating the tests to use Vite 5
- Make this PR backwards compatible
What makes the most sense to everyone?
IMHO, breaking changes should be a last resort, especially if this can be
avoided with a simple if/then statement.
…On Sun, Jan 7, 2024, 8:22 PM Jack Steam ***@***.***> wrote:
***@***.**** commented on this pull request.
@ale-ben <https://github.com/ale-ben> Thanks for the PR! Awesome research
and deduction to get this working with Vite 5! 🌟
It looks like this is a breaking change for Vite 4, but maybe that's not a
big deal? We could release this in one of two ways:
1. Major version bump and only support Vite 5+
2. Make this PR backwards compatible
What makes the most sense to everyone?
I'm going to promote the beta to latest on NPM in the next few weeks, so
maybe option one is most maintainable option.
—
Reply to this email directly, view it on GitHub
<#841 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAOLIB6EOULOHZW6OT5DWDYNLRXJAVCNFSM6AAAAABAJWPOLWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMBXHA2TCMJXGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@kaminskypavel I Agree on the braking change thing. Didn't realize that was a breaking change between 4 and 5... chrome-extension-tools/packages/vite-plugin/src/node/plugin-webAccessibleResources.ts Lines 105 to 108 in 20faa15
The idea is to have this load 'manifest.json' if Vite is v4 and '.vite/manifest.json' if vite is v5, correct? I'll finish testing the required fix and then I'll push it |
This should provide compatibility for both Vite <=4 and Vite >4. Reviewing my old commit and updating changeset I noticed that I previously considered this change as a patch. Is this correct or should it be bumped to minor? |
@jacksteamdev can we merge this? it is blocking vite upgrade |
It seems like the tests are broken, but I don't think it's this PR. I'm working to fix them now, will update later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This should solve #836
According to Vite documentation:
Right now, when building, the plugin tries to find a vite manifest without considering the
.vite
folder:chrome-extension-tools/packages/vite-plugin/src/node/plugin-webAccessibleResources.ts
Lines 105 to 108 in 20faa15
As suggested here (#836 (comment)), replacing
manifest.json
with.vite/manifest.json
solves the build error.