[core] Avoid popper.js npm warning #20433
Merged
Conversation
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
oliviertassinari
force-pushed
the
fix-popper-warning
branch
from
10c6b08 to
54ecbb0
Compare
oliviertassinari
force-pushed
the
fix-popper-warning
branch
from
54ecbb0 to
35682fd
Compare
Details of bundle changes.Comparing: 52b80a0...e800daf Details of page changes
|
oliviertassinari
added
internal
NMinhNguyen
approved these changes
Apr 5, 2020
|
I thought about a solution, I published |
I would prefer this solution. That preserves the import identifier which might be relevant for aliases. |
|
@FezVrasta Awesome, this sounds even better, updating. |
oliviertassinari
changed the title
This was referenced Apr 13, 2020
This was referenced Apr 20, 2020
This was referenced May 4, 2020
Closed
|
Something is off with the current solution: npm
yarn
It resolves to: What about this diff? diff --git a/packages/material-ui/package.json b/packages/material-ui/package.json
index 2d5560541..38d4d3fbc 100644
--- a/packages/material-ui/package.json
+++ b/packages/material-ui/package.json
@@ -56,7 +56,7 @@
"@types/react-transition-group": "^4.2.0",
"clsx": "^1.0.4",
"hoist-non-react-statics": "^3.3.2",
- "popper.js": "^1.16.1-lts",
+ "popper.js": "1.16.1-lts",
"prop-types": "^15.7.2",
"react-is": "^16.8.0",
"react-transition-group": "^4.4.0"cc @dandv |
|
@oliviertassinari that should work according to https://semver.npmjs.com/ I think the only way for the caret to not pick it up is if it was released as |
oliviertassinari
added a commit
to oliviertassinari/material-ui
that referenced
this pull request
May 30, 2020
A second iteration mui#20433 (comment).
|
FWIW this is an unfortunate diversion from semver. Using the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
I propose that we temporarily fork popper.js to avoid this warning. The fork is hosted at https://github.com/mui-org/popper-core.
I think that popper.js was right to make this deprecation, However, the concern for us is different: users have no leverage, we force it on them.
Solving the npm warnings is hard enough, Material-UI doesn't need to force this one on users. I think that we should educate users (give a feedback loop) that npm warnings are not to be ignored, they can harm. Displaying them a warning they can't make actionable doesn't foster this virtuous cycle where developers pay more attention to the npm warnings.
We should revisit this in v5.
Related to #19358.