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: include duplicate assets in the manifest #9928

Merged
merged 6 commits into from
Sep 22, 2022
Merged

feat: include duplicate assets in the manifest #9928

merged 6 commits into from
Sep 22, 2022

Conversation

timacdonald
Copy link
Contributor

@timacdonald timacdonald commented Aug 31, 2022

Description

Laravel has officially made Vite it's out-of-the-box bundler. Although Vite is targeted at full fledged JS applications, we have made some affordances to make it work well with a completely back-end templated language, such as PHP / Blade.

This PR attempts to fix an issue that is present in an entirely back-end templated application.

Here is the scenario:

You have two duplicate files on disk.

resources/images/client1/icon.png
resources/images/client2/icon.png
resources/images/client3/icon.png <= duplicate of client1/icon.png

In one of your entry points, you do the following in order for Vite to pick up the file and add it to the build:

import.meta.glob('resources/images/**')

When you run the build, you will end up with the following manifest:

{
  "resources/images/client1/icon.png": {
    "file": "assets/icon.636a0396.png",
    "src": "resources/images/client1/icon.png"
  },
  "resources/images/client2/icon.png": {
    "file": "assets/icon.71e56d11.png",
    "src": "resources/images/client2/icon.png"
  }
}

Notice that the client3 icon has not been included in the manifest. From an entirely back-end application point of view, this is not great, as when you reference assets, you are going to reference them by their original path. For example in Blade we would do the following:

<img src="{{ Vite::asset('resources/images/client1/icon.png') }}">
<img src="{{ Vite::asset('resources/images/client2/icon.png') }}">
<img src="{{ Vite::asset('resources/images/client3/icon.png') }}">

The Vite::asset() function will search for the entry in the manifest and output the correct path. i.e. the result of the above would be:

<img src="https://example.com/assets/icon.636a0396.png">
<img src="https://example.com/assets/icon.71e56d11.png">
<img src="https://example.com/">

The last one would really throw an error, but I put it there to illustrate the problem, which is that we cannot actually find the asset in the manifest as it was never put there.

This occurs because the expected naming collision:

client1/icon.png => assets/icon.636a0396.png
client3/icon.png => assets/icon.636a0396.png

of course because these files are identical, thus we don't need to emit it and process the file twice - however from a back-end perspective it would be nice if there were two chunks in the manifest that reference this same file.

My proposed solution (which may be highly illegal 🚨 🚓) is to keep track of any duplicate assets as we are processing them and manually add them to the bundle during the generateBundle hook. I'm not sure of the possible flow on effects of this, so I will 100% need input on this.

The resulting manifest:

{
  "resources/images/client1/icon.png": {
    "file": "assets/icon.636a0396.png",
    "src": "resources/images/client1/icon.png"
  },
  "resources/images/client2/icon.png": {
    "file": "assets/icon.71e56d11.png",
    "src": "resources/images/client2/icon.png"
  },
  "resources/images/client3/icon.png": {
    "file": "assets/icon.636a0396.png",
    "src": "resources/images/client3/icon.png"
  },
}

Additional context

n/a


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@timacdonald
Copy link
Contributor Author

// cc @ElMassimo I figure this might also apply in Ruby land, so just a friendly ping in case this is useful to you or if you have any other ideas 😇

@ElMassimo
Copy link
Contributor

@timacdonald Indeed it applies to Ruby land:

It's unusual, though. Other than this bug report on a sample app, I haven't seen it in the wild.

@patak-dev
Copy link
Member

patak-dev commented Aug 31, 2022

I think this is a good solution for including in 3.1. For Vite 4, we may want to revisit how the dedupe of assets works, see discussion in:

@timacdonald maybe you could add your opinion on this subject there too.

@timacdonald
Copy link
Contributor Author

I dropped some thoughts in that issue and also referenced it in the PR description, so merging this will close that issue.

Thanks for that feedback @ElMassimo! Seems we hit it early on over in Laravel-land 😀

patak-dev
patak-dev previously approved these changes Sep 1, 2022
@bluwy bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) and removed p3-minor-bug An edge case that only affects very specific usage (priority) labels Sep 1, 2022
@timacdonald
Copy link
Contributor Author

timacdonald commented Sep 2, 2022

@patak-dev @bluwy I've just pushed some new commits to this based on the concerns you have (and @bluwy the testing you did 🤦‍♂️ my bad, sorry).

This new approach only creates changes in the manifest plugin - however, it does mean that the manifest plugin and the asset plugin are now tied together, as the manifest plugin is importing stuff from the asset plugin - which felt a little like I was blurring the boundaries there.

If there is a cleaner / preferred way to transfer the information between the plugins, please let me know.

So the asset plugin now:

  1. Keep track of duplicate assets (now with the correct filename).
  2. Exports the duplicate assets.

The manifest plugin:

  1. Imports the duplicates
  2. Puts them in the manifest
  3. Additionally I added a isDuplicate key to the manifest chunk for some user facing signalling as to what is happening. You might not want this, but I think it might be good in communicating to the developers what is happening.

The generated manifest looks like this:

{
  "resources/images/duplicate-2.svg": {
    "file": "assets/duplicate-2.7189bd4d.svg",
    "src": "resources/images/duplicate-2.svg"
  },
  "resources/images/unique-1.svg": {
    "file": "assets/unique-1.a69fede7.svg",
    "src": "resources/images/unique-1.svg"
  },
  "resources/images/unique-2.svg": {
    "file": "assets/unique-2.b15947b8.svg",
    "src": "resources/images/unique-2.svg"
  },
  "resources/js/app.js": {
    "file": "assets/app.66602e29.js",
    "src": "resources/js/app.js",
    "isEntry": true
  },
  "resources/images/duplicate-1.svg": {
    "file": "assets/duplicate-2.7189bd4d.svg",
    "src": "resources/images/duplicate-1.svg",
    "isDuplicate": true
  },
  "resources/images/duplicate-3.svg": {
    "file": "assets/duplicate-2.7189bd4d.svg",
    "src": "resources/images/duplicate-3.svg",
    "isDuplicate": true
  }
}

Note that the duplicates all share the same "file", but their "source" remains intact, which I think is nice, because then you don't lose any information and if you wanted to traverse to find "file" source, you can. If we replace the "source" as well we are losing information.

Would love to know your thoughts on all this with the new changes now in place.

@timacdonald timacdonald changed the title fix: include duplicate assets in the bundle fix: include duplicate assets in the manifest Sep 2, 2022
@timacdonald timacdonald changed the title fix: include duplicate assets in the manifest feature: include duplicate assets in the manifest Sep 2, 2022
packages/vite/src/node/plugins/asset.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/manifest.ts Outdated Show resolved Hide resolved
@timacdonald timacdonald changed the title feature: include duplicate assets in the manifest feat: include duplicate assets in the manifest Sep 5, 2022
@patak-dev patak-dev added this to the 3.2 milestone Sep 6, 2022
@patak-dev patak-dev requested a review from bluwy September 6, 2022 20:22
@patak-dev
Copy link
Member

We discussed this PR in today's team meeting and we think it is good to be merged. We are going to keep the deduplication of assets, but we think the isDuplicate is redundant information in the manifest.
Let's merge this one in Vite 3.2. We have several features and fixes waiting, so we will start it sooner this time.

@timacdonald
Copy link
Contributor Author

Thank you @patak-dev and @bluwy for your help with this one and thanks to the rest of the Vite team for taking the time to consider the PR.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

This LGTM as well! I think it would be nice to add a test for this before merging. I could help if not.

@patak-dev
Copy link
Member

@bluwy I'm going to merge this one so we can start testing it on main. If you have an idea for the tests, lets do that in another PR 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants