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

[Vite Plugin] Fix vite manifest location #841

Merged
merged 9 commits into from
Jan 10, 2024
Merged

Conversation

ale-ben
Copy link
Contributor

@ale-ben ale-ben commented Dec 6, 2023

This should solve #836

According to Vite documentation:

For production: after running vite build, a .vite/manifest.json file will be generated alongside other asset files.

Right now, when building, the plugin tries to find a vite manifest without considering the .vite folder:

const viteManifest = parseJsonAsset<ViteManifest>(
bundle,
'manifest.json',
)

As suggested here (#836 (comment)), replacing manifest.json with .vite/manifest.json solves the build error.

…nifest is in .vite/manifest.json. Fixing bug that prevented building
Copy link

changeset-bot bot commented Dec 6, 2023

🦋 Changeset detected

Latest commit: cf0758b

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

This PR includes changesets to release 1 package
Name Type
@crxjs/vite-plugin Patch

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

Copy link

vercel bot commented Dec 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
vite-plugin-docs ⬜️ Ignored (Inspect) Visit Preview Jan 10, 2024 11:04pm

@kaminskypavel
Copy link

any plans on merging?

@ale-ben
Copy link
Contributor Author

ale-ben commented Dec 18, 2023

Would be nice to have any feedback on this...
If there are problems I can fix them but I would like to see it merged as well

@kaminskypavel
Copy link

Would be nice to have any feedback on this... If there are problems I can fix them but I would like to see it merged as well

this will allow upgrading to Vite@5, am I correct?

@ale-ben
Copy link
Contributor Author

ale-ben commented Dec 18, 2023

Honestly, I have no idea...
I just had the same problems described here #836 and, after reading a bit of Vite documentation, I identified where the problem was and fixed it (I think so at least)

@ale-ben
Copy link
Contributor Author

ale-ben commented Dec 19, 2023

@kaminskypavel Just noticed something, I am already using Vite@5...

Screenshot 2023-12-19 at 09 58 00

@kaminskypavel
Copy link

@jacksteamdev @AmySteam what do you guys think?

@nandanito
Copy link

@ale-ben I'm still getting the same following error, while using vite@5.0.10 with vite-plugin@2.0.0-beta.21.
[crx:web-accessible-resources] TypeError: OutputBundle["manifest.json"] is undefined.

The upgrading to 5.0.x is not working.

@ale-ben
Copy link
Contributor Author

ale-ben commented Dec 20, 2023

@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:
Vite plugin:
Screenshot 2023-12-20 at 10 25 35
My project (with dist folder manually replaced with build output):
Screenshot 2023-12-20 at 10 25 45

Can you provide an example of when it doesn't work?

@khs6666

This comment was marked as off-topic.

@nandanito
Copy link

@ale-ben Here is the repo where it is not working.

I think merging your commit in the next release might solve the issue (also discussed here).

Copy link
Contributor

@jacksteamdev jacksteamdev left a 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:

  1. Major version bump and only support Vite 5... This will require updating the tests to use Vite 5
  2. Make this PR backwards compatible

What makes the most sense to everyone?

@jacksteamdev jacksteamdev self-assigned this Jan 7, 2024
@kaminskypavel
Copy link

kaminskypavel commented Jan 7, 2024 via email

@ale-ben
Copy link
Contributor Author

ale-ben commented Jan 8, 2024

@kaminskypavel I Agree on the braking change thing. Didn't realize that was a breaking change between 4 and 5...

const viteManifest = parseJsonAsset<ViteManifest>(
bundle,
'manifest.json',
)

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

@ale-ben
Copy link
Contributor Author

ale-ben commented Jan 8, 2024

This should provide compatibility for both Vite <=4 and Vite >4.
Let me know if I missed something.

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?
Is this considered a new feature or a bug? 😅

@kaminskypavel
Copy link

@jacksteamdev can we merge this? it is blocking vite upgrade

@jacksteamdev
Copy link
Contributor

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.

Copy link
Contributor

@jacksteamdev jacksteamdev left a comment

Choose a reason for hiding this comment

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

LGTM!

@jacksteamdev jacksteamdev merged commit 48d8c68 into crxjs:main Jan 10, 2024
4 checks passed
@khs6666

This comment was marked as spam.

@khs6666

This comment was marked as spam.

@khs6666

This comment was marked as spam.

@khs6666

This comment was marked as spam.

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.

None yet

5 participants