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

Add ? to resourceQuery for Consistency with interpolateName Behavior #102

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

oe
Copy link

@oe oe commented Jan 7, 2025

This PR introduces a modification to the resourceQuery behavior by adding support for ?, ensuring consistency with the behavior outlined in webpack/loader-utils#interpolateName.

Additionally, this PR resolves a specific issue related to optional query parameters in include matching rules.

Details:

  1. Feature Addition:

    • Added support for ? in resourceQuery.
    • Aligns resourceQuery handling with the behavior of the interpolateName function, improving predictability and usability for developers.
  2. Issue Fix:

    • Fixed an issue where optional query parameters in include rules caused incorrect file name generation.
      • For example, when using an include rule like /\.png(\?.+)?/ and name value [name].[ext]?[query]:
        • A file reference like "xxx.png" previously generated an invalid file name: xxx.png?.
        • After this PR, modifying name to [name].[ext][query] resolves the issue and produces the correct output.

Breaking Changes:

This PR introduces a non-backward-compatible change by no longer supporting configurations relying on the previously incorrect file name generation (xxx.png?). Users with such configurations will need to update their name value to [name].[ext][query].

Motivation:

Ensuring consistency across related functionalities and resolving file name generation issues enhances the developer experience and eliminates potential confusion.

@coder-layne coder-layne self-requested a review January 7, 2025 15:17
@coder-layne coder-layne self-assigned this Jan 7, 2025
@coder-layne coder-layne added bug Something isn't working good first issue Good for newcomers labels Jan 7, 2025
@oe
Copy link
Author

oe commented Jan 8, 2025

Another commit has been submitted

Fix Data Corruption Issue for Binary Resources

Details:

  1. Bug Fix:

    • Fixed an issue where binary data was mishandled, leading to corrupted output files.
    • This problem primarily affects the processing of image files, although it may impact other binary resources as well.
  2. Example Update:

    • Updated the example code to include a test case for verifying the handling of binary files.
    • To reproduce the original issue for testing purposes, revert the changes made to the file src/index.ts at line 84.

Steps to Verify the Fix:

  1. Restore the previous code at src/index.ts:84 to reproduce the binary corruption issue.
  2. Use the updated example to test the fix by processing binary resources like images.
  3. Confirm that the output files are no longer corrupted and function as expected.

Motivation:

Ensuring correct handling of binary data is critical for the plugin's reliability, particularly for workflows involving binary resources such as images.

Copy link
Contributor

@coder-layne coder-layne left a comment

Choose a reason for hiding this comment

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

LGTM

@coder-layne
Copy link
Contributor

@oe Thanks for your awesome contribution! Can you use Volta to lock the toolchain version to avoid CI check failures?

@oe
Copy link
Author

oe commented Jan 8, 2025

just locked pnpm version to 9.10.0 via volta

@coder-layne
Copy link
Contributor

@oe Thank you so much! I'll merge the PR and release version v1.0.0 to prevent automatic upgrades to breaking changes.

@coder-layne coder-layne merged commit d84b582 into laynezh:main Jan 8, 2025
0 of 7 checks passed
@oe
Copy link
Author

oe commented Jan 8, 2025

Awesome!
The recent CI build error occurred because packageManager was not updated. A quick read through the ni source code revealed that packageManager is used instead of Volta. Maintaining the same information in two places can easily lead to errors :(

@coder-layne
Copy link
Contributor

@oe I totally agree with you, but during local testing, I noticed that ni doesn't follow the version configured in the packageManager field. We could consider using Volta in CI to lock the toolchain version in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants