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: exclude assets with ?raw and ?url queries from processing #750

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

didoin
Copy link

@didoin didoin commented Sep 27, 2024

  • Quick Checklist
  • I have read the contributing guidelines
  • I have written new tests, as applicable (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • I have added a changeset, if applicable
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    This fixes the bug discussed in The plugin processes images with ?raw and ?url queries when it shouldn't #748.

  • What is the new behavior (if this is a feature change)?

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this
    PR?)
    No.

  • Other information:

Copy link

changeset-bot bot commented Sep 27, 2024

⚠️ No Changeset found

Latest commit: b67d1c7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@benmccann
Copy link
Collaborator

I wonder if it should be something more like \?(.*&)?(url|raw) in case it is combined with another query parameter. Probably unlikely, but maybe safer?

@didoin
Copy link
Author

didoin commented Sep 27, 2024

The official Vite documentation explicitly talks about ?url suffix and ?raw suffix.
Static Asset Handling

Also, I think your pattern doesn't allow specifying future query parameters (perhaps for new transformers) that are prefixed with raw or url (?rawable example, I know it's a bad example 😂).

@JonasKruckenberg
Copy link
Owner

JonasKruckenberg commented Oct 6, 2024

specifying future query parameters (perhaps for new transformers) that are prefixed with raw or url

For what its worth, I think we should not worry about that right now, if a situation arises in the future where there are more special query params like that we can easily push another version then.

@didoin
Copy link
Author

didoin commented Oct 6, 2024

If you feel that a more rigorous regular expression would be more appropriate, that's perfectly fine. From my perspective, though, it might not be necessary in this case, since "vite" expects these parameters as a suffix.

As for the "future query parameters" concern, I was also considering the possibility of custom parameters that users might define through the extendTransforms function. It seems like we could avoid potential issues down the line by addressing this now.

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.

3 participants