[test] Deprecate /test-utils#24074
Closed
oliviertassinari wants to merge 2 commits intomui:v4-deprecationsfrom
Closed
[test] Deprecate /test-utils#24074oliviertassinari wants to merge 2 commits intomui:v4-deprecationsfrom
oliviertassinari wants to merge 2 commits intomui:v4-deprecationsfrom
Conversation
|
@material-ui/core: parsed: +Infinity% , gzip: +Infinity% Details of bundle changes.Comparing: c7c52eb...d643412 Details of page changes
|
de9d6c7 to
73e2614
Compare
42 tasks
eps1lon
requested changes
Dec 21, 2020
|
|
||
| // Generate an enhanced mount function. | ||
| export default function createMount(options = {}) { | ||
| if (!warnedOnce && process.env.INTERNAL !== 'true') { |
Member
There was a problem hiding this comment.
Do we even use these internally? I thought I completely forked them in test/utils. Could you follow the breaking approach approach in the breaking change instead so that we don't pollute the environment with more variables?
Member
Author
There was a problem hiding this comment.
It seems to depend on the test utils. I could find two used:
Could you follow the breaking approach
Could you expand on this approach? I don't follow what you are referring to.
Member
There was a problem hiding this comment.
For deprecations the simplest approach is to cherry-pick the breaking change and then revert all the breaking changes to the public API. In this case:
- cherry-pick [core] Remove /test-utils #21855
- Add a deprecation warning to methods in
'@material-ui/core/test-utils'instead of removing them
Conceptually we want to move from master to next. It is easier for now (and for feature integration of master into next) to re-use as much as possible from the commit that made master and next diverge. The goal is to move master and next "closer" together. Your approach is moving them farther apart making it harder to maintain both branches.
Member
Author
There was a problem hiding this comment.
I see, thanks for the explanation. You are right, optimizing for the gap between master and next was not something I took into account in the first attempt. I was going for these two objectives:
- keeping the
masterandv4-deprecationsbranches as close as possible so we can mergev4-deprecationsback intomastereasily once we are done with all the deprecations. - spend as little as time as I could get away with it (aiming for the good enough, or almost good enough)
Member
Author
There was a problem hiding this comment.
I have looked at the path proposed, we would need to resolve these conflicts:
I have a proposal to move forward. If temporarily not having an extra variable in the test env important for you, we can close this PR. You resume the effort later on with the alternative you proposed #24074 (comment). Otherwise, we can move forward as is, to save time. I personally don't want to spend time resolving 74 conflicts for the outcome we could get in exchange. What do you think? This way you can control the quality of the outcome, nor do we need to rush it :).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add deprecations for #21855.