Skip to content

Support preserving the inlined asset #434

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

Merged
merged 3 commits into from
Jun 2, 2023
Merged

Support preserving the inlined asset #434

merged 3 commits into from
Jun 2, 2023

Conversation

SorsOps
Copy link
Contributor

@SorsOps SorsOps commented Jun 1, 2023

Description

Adds a filter to support whether or not an asset should be deleted from the output.

This has a niche application where we needed to preserve the bundled script in the output to upload to another server in addition to having it inlined.

How has this been tested?

Added a test for it

Types of changes

  • New feature - feat
  • Bug fix - fix
  • Refactor - refactor
  • Test cases - test
  • Other(s):

Remarks

N/A

@SorsOps
Copy link
Contributor Author

SorsOps commented Jun 1, 2023

@icelam Let me know if there are any changes needed

@icelam
Copy link
Owner

icelam commented Jun 2, 2023

Hi @SorsOps, thank you for submitting this Pull Request, your addition of a new feature is appreciated.

How do you feel about changing preserveAsset: (assetName: string) => boolean to assetsPreservePattern: RegExp[]? I understand that using a function allows user to have more control on what assets needs to be preserved. However, consider that both existing parameters htmlMatchPattern and scriptMatchPattern are having type RegExp[], it will be more consistent and intuitive to user that preserveAsset will also accept an array of regex. I did consider changing the existing parameters to accept the feature, but I don't want users to have to tweak their webpack config when they upgrade this plugin.

We might just need to add a utility function that is similar to the existing isFileNeedsToBeInlined to achieve the same result.

@SorsOps
Copy link
Contributor Author

SorsOps commented Jun 2, 2023

@icelam Makes sense. I've made the recommended changes

@icelam
Copy link
Owner

icelam commented Jun 2, 2023

@SorsOps Thank you for updating your PR. The update looks good to me. I just have one minor comment that I would like to rename preserveAsset to assetPreservePattern so that it sync with other variable names. To speed up the process, allow me to co-author this PR. I will release a new version of package once I merge this PR and update the README.

Have a nice weekend :)

@icelam icelam merged commit 239e7b9 into icelam:develop Jun 2, 2023
@icelam
Copy link
Owner

icelam commented Jun 2, 2023

@SorsOps I have published v3.2.0 which includes this additional feature! Let me know if it has any issue.

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.

2 participants