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: Add filename normalizing #214

Merged
merged 8 commits into from
Sep 9, 2020
Merged

feat: Add filename normalizing #214

merged 8 commits into from
Sep 9, 2020

Conversation

beeequeue
Copy link
Contributor

What kind of change does this PR introduce?

Feature

Did you add tests for your changes?

I added some tests for the normalizing, but couldn't find any way to test it further than I did. Please let me know if anything else should be added.

If relevant, link to documentation update:

TODO

Summary

Some bundles may have hashes in their filenames - this breaks comparisons since files appear to be deleted instead of modified when the hashes change.

This adds an option for normalizing these filenames to fix this iseue.

I also thought about adding an enum option, so you can easily remove hashes without having to write a new Regex for it.

e.g.

$ yarn bundlewatch --normalize hash

Does this PR introduce a breaking change?

No

Other information

While adding the config, I made the validators able to alter the configuration to be valid to easily parse string regexes from JSON files or CLI options.

@beeequeue beeequeue changed the title Filename sanitizing feat: Add filename normalizing Aug 12, 2020
@beeequeue
Copy link
Contributor Author

Just saw #192, oops.

@beeequeue beeequeue closed this Aug 12, 2020
@cheapsteak
Copy link
Contributor

cheapsteak commented Aug 16, 2020

@beeequeue yours looks like a fine solution, you also don't have merge conflicts on the branch I think?
might as well reopen this/leave it up and see which one @jakebolam might prefer XD

@beeequeue beeequeue reopened this Aug 16, 2020
@jakebolam
Copy link
Member

Thank you for doing this, this is an awesome feature.

@jakebolam
Copy link
Member

I'll get this merged, any chance you want to add some docs for this?
https://github.com/bundlewatch/bundlewatch.io

@jakebolam
Copy link
Member

Fixes: #30

@jakebolam jakebolam merged commit 6b53e82 into bundlewatch:master Sep 9, 2020
@cheapsteak
Copy link
Contributor

cheapsteak commented Sep 9, 2020

🎉
@beeequeue do you happen to have an example of a cli command including the regex for removing the hash?

@cheapsteak
Copy link
Contributor

ooo nvm this worked for us (added to the config in package.json)
"normalizeFilenames": "^.+?(\\.\\w+)\\.(?:js|css)$"

sweet!!!

@beeequeue
Copy link
Contributor Author

Yes I will open a PR in the website to add documentation

@beeequeue beeequeue deleted the filename-sanitizing branch September 9, 2020 15:56
@beeequeue
Copy link
Contributor Author

@cheapsteak Could you confirm for me that this working as intended for you? I'm seeing some weird behaviour where it still says the files are deleted and re-added even though they have the same name...

Example

@cheapsteak
Copy link
Contributor

cheapsteak commented Sep 16, 2020

@beeequeue I ran into the same issue 😞 (see #30 (comment) )

@cheapsteak
Copy link
Contributor

cheapsteak commented Sep 16, 2020

my guess is that the problem might be here:

const baseBranchFileDetails = await bundlewatchService.getFileDetailsForBaseBranch()
await bundlewatchService.saveFileDetailsForCurrentBranch({
fileDetailsByPath: currentBranchFileDetails,
trackBranches: ci.trackBranches,
})
const results = analyze({
currentBranchFileDetails,
baseBranchFileDetails,
baseBranchName: ci.repoBranchBase,
normalizeFilenames,
})

The file path normalization is being done inside the analyze call, but file details are sent into saveFileDetailsForCurrentBranch before normalization/analyze runs

I think we might need to move normalization into getLocalFileDetails instead

@cheapsteak
Copy link
Contributor

@jakebolam @beeequeue #238 should fix this (no api changes)

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

3 participants