Take 2 of the require.resolve change. This time using the "resolve" pkg#5031
Take 2 of the require.resolve change. This time using the "resolve" pkg#5031kenotron merged 5 commits intomicrosoft:masterfrom
Conversation
cliffkoh
left a comment
cliffkoh
left a comment
There was a problem hiding this comment.
I haven't looked super closely since it is really late, but we shouldn't be taking an implicit dependency like this.. if we depend on a package, we should explicitly specify it as a dependency.
(Amongst other things, I'm 95% sure rush for example also rely on proper use of NPM semantics like this in its symlinking algo/strategy. @pgonzal can correct me if I am mistaken here :))
| "packageName": "@uifabric/fabric-website", | ||
| "comment": "revert build change", | ||
| "type": "none" | ||
| "comment": "made sure the deps are resolved by package rather than relative paths", |
There was a problem hiding this comment.
Uhh this might cause problems. You should avoid the deletion of the revert change file as part of your change here.. If you uncommit the deletion then merge master, it will show up as a new change file rather than a rename.
There was a problem hiding this comment.
Oh I assumed that we merge the changes like this all in one go. I can revert the deletion
|
One other reason taking an implicit dependency is bad is that the You're presently relying on the fact that the present version preserves symlinks by default , but the next major version of (Of course, the best thing to do here isn't necessarily to lock to a particular version per se, but to explicitly specify the symlink preservation behavior you want in the opts argument). |
* master: (95 commits) Applying package updates. Experiments/Nav component: display "show more" link only if there is atleast one hidden link (microsoft#5057) Add pointerup listener to exit keytip mode (microsoft#5051) Update PULL_REQUEST_TEMPLATE.md Update ISSUE_TEMPLATE.md Shimmer: resolve conflicts (microsoft#5034) Invalid ARIA attributes: Fix empty values (microsoft#5040) ComboBox: Correct invalid ARIA attributes. (microsoft#4873) (microsoft#5001) ComboBox: Fix submit pending value (microsoft#5048) FocusTrapZone - restore last focused descendant element (microsoft#4897) Applying package updates. Take 2 of the require.resolve change. This time using the "resolve" pkg (microsoft#5031) fixing webpack config to allow rush build to complete on a small VM (microsoft#5037) MessageBar: change color of X close button so that it is accessible (microsoft#5039) Theming: improve accessibility (microsoft#5038) Applying package updates. Added 'made with fabric' to readme (microsoft#5018) HoverCard: example accessibility fix. (microsoft#5027) Dropdown caret (microsoft#5016) Applying package updates. ...
* master: (274 commits) Demo Page Refactor Part 1 (microsoft#5055) SplitButton: Add aria-roledescription (microsoft#5062) Add addins sketch toolkit link (microsoft#5052) Dropdown title (microsoft#4983) Allow for more control over event handling for keytips (microsoft#5064) Build time speed improvements (microsoft#4965) Coachmark: Positioning fixes (microsoft#4995) Applying package updates. Experiments/Nav component: display "show more" link only if there is atleast one hidden link (microsoft#5057) Add pointerup listener to exit keytip mode (microsoft#5051) Update PULL_REQUEST_TEMPLATE.md Update ISSUE_TEMPLATE.md Shimmer: resolve conflicts (microsoft#5034) Invalid ARIA attributes: Fix empty values (microsoft#5040) ComboBox: Correct invalid ARIA attributes. (microsoft#4873) (microsoft#5001) ComboBox: Fix submit pending value (microsoft#5048) FocusTrapZone - restore last focused descendant element (microsoft#4897) Applying package updates. Take 2 of the require.resolve change. This time using the "resolve" pkg (microsoft#5031) fixing webpack config to allow rush build to complete on a small VM (microsoft#5037) ...
2 participants
Pull request checklist
$ npm run changeDescription of changes
Same as last time when I tried to do the
require.resolvechange. However, node require.resolve seem to give inconsistent results with rush'ed node_modules it seems to have issues with symlinks, etc. Using theresolvepackage that is already included by thecpxdependency solves the issue.Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow