Create-package fixes and related updates #10212
Conversation
ecraig12345
requested review from
aditima,
dzearing,
evlevy,
Jahnp,
JasonGore,
KatherineThayerMicrosoft,
kenotron,
KevinTCoughlin,
Raghurk and
a team
as code owners
|
Size Auditor did not detect a change in bundle size for any component! |
|
Component Perf Analysis:
|
| "react-dom": "{{{reactDom}}}" | ||
| "@types/react": "{{{typesReactPeerDep}}}", | ||
| "@types/react-dom": "{{{typesReactDomPeerDep}}}", | ||
| "react": "{{{reactPeerDep}}}", |
There was a problem hiding this comment.
I don't think we really need to do the @types/* peer deps anymore. I think that was related to a rush update fix.
There was a problem hiding this comment.
Okay so should I revert them all now? Change was made in #9798 for reference. It actually seems like a reasonable change since if people want to use OUFR with TS, they'll need React typings.
There was a problem hiding this comment.
I don't think this is a Rush related fix, but rather a Fabric + TypeScript related fix that we should keep. Assuming new packages spun up leverage React (likely) then including them in the package.json template seems reasonable.
There was a problem hiding this comment.
@kenotron As explained in #9720 (comment), if your package imports stuff from another package, you need to declare that dependency in your package.json. If you don't declare it, then complex installations /will/ malfunction.
Rush users tend to encounter these problems more often, because Rush's audience tends to be running serious production monorepos with complex installations. Also, Rush supports validation checks to proactively detect some of these problems. Nonetheless, the problem applies to every package manager: No package manager can perform a correct side-by-side installation for dependencies that were undeclared.
There was a problem hiding this comment.
Ken pointed out an interesting case, what about our consumers who don't use TypeScript? It's not ideal to make them either depend on @types/react and @types/react-dom or have unmet peer dependencies. But I also see the value of enforcing the correct typing versions for those consumers who do use TypeScript.
There was a problem hiding this comment.
The rush update fixed everything because it inadvertently upgraded to office-ui-fabric-react 5.135.3 with the peer dependencies added. 😁
There was a problem hiding this comment.
Okay, I have locked the version to that original versions... Still builds fine...
There was a problem hiding this comment.
This fix works because although you locked the direct dependency on office-ui-fabric-react to "5.113.1", the indirect dependencies still got upgraded. For example, @uifabric/utilities was solved as 5.34.3 which has the @types/react peer. Because office-ui-fabric-react does not satisfy this peer, it gets determined by the parent project test-15. This induces an implicit peer dependency for [email protected] which is recorded in the lockfile like this:
/office-ui-fabric-react/5.113.1_ee63188b739fb4d6a436db584e63e7a3:
dependencies:
'@microsoft/load-themed-styles': 1.9.19
'@uifabric/icons': 5.8.0_ee63188b739fb4d6a436db584e63e7a3
'@uifabric/merge-styles': 5.17.1
'@uifabric/styling': 5.37.0_ee63188b739fb4d6a436db584e63e7a3
'@uifabric/utilities': 5.34.3_ee63188b739fb4d6a436db584e63e7a3
prop-types: 15.7.2
react: 15.6.2
react-dom: [email protected]
tslib: 1.10.0
dev: false
peerDependencies:
'@types/react': '*'
'@types/react-dom': '*'
react: ^0.14.9 || ^15.0.1-0 || ^16.0.0-0
react-dom: ^0.14.9 || ^15.0.1-0 || ^16.0.0-0In order to really test your theory, a reliable way would be to add some code like this to common/config/rush/pnpmfile.js:
/**
* This hook is invoked during installation before a package's dependencies
* are selected.
* The `packageJson` parameter is the deserialized package.json
* contents for the package that is about to be installed.
* The `context` parameter provides a log() function.
* The return value is the updated object.
*/
function readPackage(packageJson, context) {
if (packageJson.peerDependencies) {
delete packageJson.peerDependencies['@types/react'];
delete packageJson.peerDependencies['@types/react-dom'];
}
return packageJson;
}During rush install, this will revert the fix where we added peerDependencies, for all packages including indirect dependencies. When I tried that and ran rush update --full, the build failure repros again.
There was a problem hiding this comment.
I see now the issue isn't OUFR, it's actually the internal deps of OUFR from our own repo that needs @types/react. When I did tsc, it gives this sort of result:
../../common/temp/node_modules/.registry.npmjs.org/@uifabric/utilities/[email protected][email protected]/node_modules/@uifabric/utilities/lib-es2015/BaseComponent.d.ts:1:24 - error TS7016: Could not find a declaration file for module 'react'. '/Users/ken/workspace/harry/common/temp/node_modules/.registry.npmjs.org/react/15.6.2/node_modules/react/react.js' implicitly has an 'any' type.
If the 'react' package actually exposes this module, consider sending a pull request to amend 'https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react`
1 import * as React from 'react';
So pnpm resolves these really specific dirs for its npm 2 style directories. I just realized that pnpm actually CHANGES the shape of this by the peer dependency hints that we give when a conflict arises in a side-by-side environment (strange, but not unheard of in a more complex repo). Inspired by your last proposal, I took a stab at solving the OP's issue with a pnpmfile.js change like this:
/**
* This hook is invoked during installation before a package's dependencies
* are selected.
* The `packageJson` parameter is the deserialized package.json
* contents for the package that is about to be installed.
* The `context` parameter provides a log() function.
* The return value is the updated object.
*/
function readPackage(packageJson, context) {
if (packageJson.name === 'office-ui-fabric-react' || packageJson.name.startsWith('@uifabric')) {
packageJson.peerDependencies['@types/react'] = '*';
packageJson.peerDependencies['@types/react-dom'] = '*';
}
return packageJson;
}I'm not sure how often we have UI Fabric deps installed side-by-side by our consumers. This might be acceptable as a workaround for them, but I think if all it takes is put some hints in our package.json, it's not too bad. This has been a pretty interesting education for me on pnpm and how it treats things - kudos to the fact that you can programmatically change how it sees package.json.
Conclusion - keep this in the create-package script. It's a good change.
There was a problem hiding this comment.
"Hints" isn't best term for these peer dependencies. It's better to think of them as fixes for real bugs in package.json. Side-by-side OUIFR versions seems like an esoteric edge case, but it was just an easy example. It's the tip of an iceberg. Wherever package.json bugs are found, this same problem arises over and over and over again in a myriad of variations. It's a real headache for developers. As participants in the NPM ecosystem, we all can work together and do our part to eliminate these bugs.
Although this investigation focused on PNPM, the same thing will repro easily with Yarn or NPM or even Yarn Plug'n'Play. Their lockfiles are radically different, but the fix is always the same: Declare the missing deps.
So pnpm resolves these really specific dirs for its npm 2 style directories.
Heh, I suppose the deep node_modules paths are a bit reminiscent of "NPM 2". It's only a superficial similarity, though. PNPM invented a completely novel installation plan that solves the same important problems that Yarn recently tackled with their Plug'n'Play feature. Whereas Plug'n'Play relies on redefining NodeJS's module resolution behavior, PNPM instead installs a node_modules folder with lots of nesting symlinks. This weird symlinking strategy enabled PNPM to be backwards-compatible with existing NodeJS module resolution, which is how it was able to predate Plug'n'Play by a couple years.
Likening it to NPM 2 suggests that PNPM is old fashioned, but NPM 6 doesn't solve these problems at all. (The specific problems I'm referring to are doppelgangers and phantom dependencies.)
Conclusion - keep this in the create-package script. It's a good change.
Yay, thanks! 😊
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
Pull request checklist
$ yarn changeDescription of changes
Fix some issues with create-package:
For some reason it was creating two Jest setup files,
config/tests.jsandsrc/common/tests.js. Onlyconfig/tests.jsever seems to be referenced, so delete the other one various places and delete the line in create-package that creates it.Don't create empty .vscode directories
Use the correct version ranges for react/react-dom peer deps, and add the corresponding types as peer deps
Update the npmignore to be consistent across packages (with a couple exceptions)
Microsoft Reviewers: Open in CodeFlow