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 a syntax for adjusting merge behavior without matching #152

Merged
merged 5 commits into from
Dec 10, 2020

Conversation

bebraw
Copy link
Member

@bebraw bebraw commented Oct 16, 2020

@dennisoelkers This is my go at fixing your issue. I set up tests based on your code and fixed one of the problems.

I think the resolve.extensions case needs a bit more to go with it. Do you have more complex configuration there in your case? The trivial way to solve it would be to use resolve: 'replace' but that would work only for this specific case. Now the design is that it's going to append extensions only if a sibling matches which sort of makes sense logically.

Closes #151.

@bebraw
Copy link
Member Author

bebraw commented Oct 22, 2020

@dennisoelkers Can you provide more complex configuration to test against for you resolve.extensions case? The first part of your issue is easy to fix but I'll need to find some way to handle the rest. One possible option would be to add new operation or so to handle it.

@dennisoelkers
Copy link

@bebraw: If I understood correctly, a possible use case would be merging two disjunct arrays:

const config1 = { resolve: { extensions: ['.js'] } };
const config2 = { resolve: { extensions: ['.ts'] } };

merge(config1, config2);

(Runnable example in https://runkit.com/embed/wibzpdx8gmug)

It returns an empty array as well and should return the superset of both arrays instead.

@bebraw
Copy link
Member Author

bebraw commented Oct 22, 2020

@dennisoelkers Thanks. That's a good example and I see the use case now. In the current logic it's applying append/prepend/replace against the same level within the object but I see this has to be expanded.

One interesting option might be to support syntax like extensions: ['match', 'append'], or I'll add some logic so that if there's a match, it will check the children as well (or child checks if parent had a match). I'm leaning towards latter.

@bebraw
Copy link
Member Author

bebraw commented Nov 4, 2020

@dennisoelkers I pushed an experimental syntax to the PR after thinking about the issue for a while. Now there are things like AppendOnly that work without a Match. I'm not sure if that's the final naming but it would be great if you could have a look to see if that covers your use case.

@codecov-io
Copy link

codecov-io commented Nov 4, 2020

Codecov Report

Merging #152 (74ef999) into develop (105007e) will decrease coverage by 0.30%.
The diff coverage is 95.23%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #152      +/-   ##
===========================================
- Coverage    98.47%   98.17%   -0.31%     
===========================================
  Files            6        6              
  Lines          197      219      +22     
  Branches        55       60       +5     
===========================================
+ Hits           194      215      +21     
- Misses           3        4       +1     
Impacted Files Coverage Δ
src/index.ts 97.88% <93.33%> (-0.56%) ⬇️
src/types.ts 100.00% <100.00%> (ø)
src/utils.ts 100.00% <100.00%> (ø)
src/unique.ts 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 105007e...ef16b6e. Read the comment docs.

@bebraw
Copy link
Member Author

bebraw commented Nov 12, 2020

@dennisoelkers FYI, I just rebased the PR so it merges well. I think it's going to need better naming somehow (Only doesn't feel right) and likely I have to add a few more tests to exercise possible mixed cases better.

@bebraw bebraw changed the title fix - Fix a throwing case for mergeRules feat - Add a syntax for adjusting merge behavior without matching Nov 13, 2020
src/index.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
test/merge-with-rules.test.ts Outdated Show resolved Hide resolved
@just-jeb
Copy link
Contributor

Hey @bebraw, do you have any ETA for this? Thanks!

@bebraw
Copy link
Member Author

bebraw commented Nov 22, 2020 via email

@just-jeb
Copy link
Contributor

just-jeb commented Dec 8, 2020

Hey @bebraw, any updates on this?

@bebraw
Copy link
Member Author

bebraw commented Dec 8, 2020 via email

@bebraw
Copy link
Member Author

bebraw commented Dec 9, 2020

@just-jeb This one is good for a look. As the implementation is quite simple, I wonder if I missed something obvious. Likely a couple more tests wouldn't hurt but API-wise it's definitely better now.

Note that I might have to migrate away from Travis as they've added significant restrictions for open source usage so it's possible the CI task won't complete.

Copy link
Contributor

@just-jeb just-jeb left a comment

Choose a reason for hiding this comment

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

LGTM

@bebraw bebraw merged commit c2e79a0 into develop Dec 10, 2020
@bebraw
Copy link
Member Author

bebraw commented Dec 10, 2020

Included to 5.5.0.

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.

Merging complete webpack configs using mergeWithRules empties primitive arrays
4 participants