chore(tools): bootstrap nx workpsace-tools and create workspace generator #18194
Conversation
Hotell
force-pushed
the
hotell/build-system/nx-workspace-generators-setup
branch
from
45a0722 to
a82fe97
Compare
| "@nrwl/jest": "12.1.0", | ||
| "@nrwl/node": "12.1.0", | ||
| "@nrwl/tao": "12.1.0", | ||
| "@nrwl/workspace": "12.1.0", |
There was a problem hiding this comment.
reordered by yarn add
new installed packages:
"@nrwl/devkit": "12.1.0",
"@nrwl/jest": "12.1.0",
"@nrwl/node": "12.1.0",
tslib": "2.2.0 --> added to root (single version policy)
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 975140887c6ad2391f8db63003ed440239ec9c25 (build) |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e8f7adc:
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 972 | 970 | 5000 | |
| BaseButton | mount | 995 | 1052 | 5000 | |
| Breadcrumb | mount | 2798 | 2826 | 1000 | |
| ButtonNext | mount | 571 | 586 | 5000 | |
| Checkbox | mount | 1820 | 1775 | 5000 | |
| CheckboxBase | mount | 1478 | 1467 | 5000 | |
| ChoiceGroup | mount | 5406 | 5350 | 5000 | |
| ComboBox | mount | 1081 | 1110 | 1000 | |
| CommandBar | mount | 11066 | 11097 | 1000 | |
| ContextualMenu | mount | 6794 | 6811 | 1000 | |
| DefaultButton | mount | 1291 | 1304 | 5000 | |
| DetailsRow | mount | 4231 | 4179 | 5000 | |
| DetailsRowFast | mount | 4151 | 4195 | 5000 | |
| DetailsRowNoStyles | mount | 3960 | 3881 | 5000 | |
| Dialog | mount | 2334 | 2302 | 1000 | |
| DocumentCardTitle | mount | 164 | 161 | 1000 | |
| Dropdown | mount | 3636 | 3658 | 5000 | |
| FocusTrapZone | mount | 1927 | 1955 | 5000 | |
| FocusZone | mount | 1921 | 1901 | 5000 | |
| IconButton | mount | 1963 | 1979 | 5000 | |
| Label | mount | 368 | 377 | 5000 | |
| Layer | mount | 2042 | 2024 | 5000 | |
| Link | mount | 536 | 522 | 5000 | |
| MakeStyles | mount | 1959 | 1943 | 50000 | |
| MenuButton | mount | 1707 | 1689 | 5000 | |
| MessageBar | mount | 2232 | 2248 | 5000 | |
| Nav | mount | 3631 | 3698 | 1000 | |
| OverflowSet | mount | 1145 | 1104 | 5000 | |
| Panel | mount | 2243 | 2240 | 1000 | |
| Persona | mount | 913 | 916 | 1000 | |
| Pivot | mount | 1571 | 1592 | 1000 | |
| PrimaryButton | mount | 1480 | 1491 | 5000 | |
| Rating | mount | 8674 | 8814 | 5000 | |
| SearchBox | mount | 1565 | 1553 | 5000 | |
| Shimmer | mount | 2890 | 2912 | 5000 | |
| Slider | mount | 2200 | 2219 | 5000 | |
| SpinButton | mount | 5519 | 5593 | 5000 | |
| Spinner | mount | 453 | 468 | 5000 | |
| SplitButton | mount | 3584 | 3587 | 5000 | |
| Stack | mount | 568 | 558 | 5000 | |
| StackWithIntrinsicChildren | mount | 1673 | 1719 | 5000 | |
| StackWithTextChildren | mount | 5252 | 5127 | 5000 | |
| SwatchColorPicker | mount | 11418 | 11285 | 5000 | |
| Tabs | mount | 1572 | 1583 | 1000 | |
| TagPicker | mount | 2675 | 2759 | 5000 | |
| TeachingBubble | mount | 12757 | 12829 | 5000 | |
| Text | mount | 447 | 480 | 5000 | |
| TextField | mount | 1546 | 1646 | 5000 | |
| ThemeProvider | mount | 1278 | 1310 | 5000 | |
| ThemeProvider | virtual-rerender | 638 | 630 | 5000 | |
| ThemeProviderNext | mount | 7011 | 7056 | 5000 | |
| Toggle | mount | 908 | 914 | 5000 | |
| buttonNative | mount | 115 | 128 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| SegmentMinimalPerf.default | 448 | 401 | 1.12:1 |
| AvatarMinimalPerf.default | 230 | 208 | 1.11:1 |
| SkeletonMinimalPerf.default | 435 | 403 | 1.08:1 |
| TreeWith60ListItems.default | 205 | 189 | 1.08:1 |
| RefMinimalPerf.default | 269 | 252 | 1.07:1 |
| GridMinimalPerf.default | 399 | 377 | 1.06:1 |
| ListCommonPerf.default | 751 | 711 | 1.06:1 |
| TextMinimalPerf.default | 409 | 386 | 1.06:1 |
| VideoMinimalPerf.default | 734 | 692 | 1.06:1 |
| BoxMinimalPerf.default | 410 | 390 | 1.05:1 |
| HeaderMinimalPerf.default | 430 | 411 | 1.05:1 |
| InputMinimalPerf.default | 1409 | 1344 | 1.05:1 |
| MenuButtonMinimalPerf.default | 1848 | 1756 | 1.05:1 |
| AttachmentSlotsPerf.default | 1317 | 1270 | 1.04:1 |
| HeaderSlotsPerf.default | 919 | 884 | 1.04:1 |
| ListMinimalPerf.default | 572 | 552 | 1.04:1 |
| AlertMinimalPerf.default | 326 | 315 | 1.03:1 |
| ButtonMinimalPerf.default | 195 | 189 | 1.03:1 |
| FlexMinimalPerf.default | 330 | 321 | 1.03:1 |
| ImageMinimalPerf.default | 435 | 424 | 1.03:1 |
| LabelMinimalPerf.default | 454 | 441 | 1.03:1 |
| ListWith60ListItems.default | 742 | 723 | 1.03:1 |
| ProviderMinimalPerf.default | 1066 | 1038 | 1.03:1 |
| StatusMinimalPerf.default | 809 | 785 | 1.03:1 |
| TableManyItemsPerf.default | 2230 | 2163 | 1.03:1 |
| TreeMinimalPerf.default | 913 | 884 | 1.03:1 |
| AnimationMinimalPerf.default | 463 | 452 | 1.02:1 |
| CheckboxMinimalPerf.default | 3014 | 2968 | 1.02:1 |
| ItemLayoutMinimalPerf.default | 1444 | 1409 | 1.02:1 |
| LayoutMinimalPerf.default | 425 | 415 | 1.02:1 |
| PopupMinimalPerf.default | 630 | 619 | 1.02:1 |
| PortalMinimalPerf.default | 182 | 178 | 1.02:1 |
| ProviderMergeThemesPerf.default | 1772 | 1738 | 1.02:1 |
| CardMinimalPerf.default | 643 | 637 | 1.01:1 |
| ChatMinimalPerf.default | 701 | 695 | 1.01:1 |
| EmbedMinimalPerf.default | 4539 | 4506 | 1.01:1 |
| FormMinimalPerf.default | 480 | 475 | 1.01:1 |
| RadioGroupMinimalPerf.default | 497 | 492 | 1.01:1 |
| SplitButtonMinimalPerf.default | 4222 | 4161 | 1.01:1 |
| CustomToolbarPrototype.default | 4143 | 4100 | 1.01:1 |
| ButtonOverridesMissPerf.default | 1821 | 1822 | 1:1 |
| ButtonSlotsPerf.default | 622 | 622 | 1:1 |
| CarouselMinimalPerf.default | 531 | 531 | 1:1 |
| DialogMinimalPerf.default | 830 | 831 | 1:1 |
| DropdownManyItemsPerf.default | 772 | 774 | 1:1 |
| ListNestedPerf.default | 659 | 660 | 1:1 |
| MenuMinimalPerf.default | 960 | 960 | 1:1 |
| IconMinimalPerf.default | 714 | 713 | 1:1 |
| TextAreaMinimalPerf.default | 568 | 569 | 1:1 |
| TooltipMinimalPerf.default | 1091 | 1090 | 1:1 |
| DatepickerMinimalPerf.default | 6109 | 6153 | 0.99:1 |
| ToolbarMinimalPerf.default | 1052 | 1062 | 0.99:1 |
| DividerMinimalPerf.default | 405 | 413 | 0.98:1 |
| DropdownMinimalPerf.default | 3277 | 3328 | 0.98:1 |
| LoaderMinimalPerf.default | 773 | 786 | 0.98:1 |
| SliderMinimalPerf.default | 1644 | 1670 | 0.98:1 |
| TableMinimalPerf.default | 462 | 473 | 0.98:1 |
| ChatDuplicateMessagesPerf.default | 335 | 347 | 0.97:1 |
| ReactionMinimalPerf.default | 430 | 442 | 0.97:1 |
| AttachmentMinimalPerf.default | 187 | 197 | 0.95:1 |
| RosterPerf.default | 1270 | 1359 | 0.93:1 |
| ChatWithPopoverPerf.default | 387 | 422 | 0.92:1 |
| AccordionMinimalPerf.default | 158 | 186 | 0.85:1 |
Hotell
force-pushed
the
hotell/build-system/nx-workspace-generators-setup
branch
2 times, most recently
from
8c2119c to
7302563
Compare
| "@nrwl/workspace": "12.1.0", | ||
| "@nrwl/cli": "12.1.0", | ||
| "@nrwl/tao": "12.1.0" | ||
| "webpack-bundle-analyzer": "4.4.2", |
There was a problem hiding this comment.
propagated packages from scripts/ to single version policy as they had chalk as a dep which was causing issues
"just-scripts": "1.3.1",
"webpack-dev-middleware": "4.2.0",
"webpack-hot-middleware": "2.25.0",
| "beachball": "1.53.1", | ||
| "chalk": "4.1.0", |
There was a problem hiding this comment.
bumped chalk to 4.1.0
-> after I added additional @nrwl/* packages (they have chalk 4.1 as dependency),
build started to fail
-> because react-conformance still depends on old chalk 2 ( which did lot of mistakes regarding exposing APIs for js and ts consumers), so scripts/exec was loading chalk 4.1 but react-conformance was supposed to consume 2.x but because you cannot consume 2 versions of the same package without hacks (aliasing), react-conformance got v4 which has different API and thus failed build
| "ts-jest": "24.3.0", | ||
| "ts-loader": "8.0.14", | ||
| "tsconfig-paths-webpack-plugin": "3.5.1", | ||
| "tslib": "2.2.0", |
There was a problem hiding this comment.
propagated tslib to single version policy for devtooling
package.json
Outdated
| @@ -176,7 +184,7 @@ | |||
| }, | |||
| "syncpack": { | |||
| "prod": true, | |||
| "dev": true, | |||
| "dev": false, | |||
There was a problem hiding this comment.
I turned off checks for devDeps. This tool wont work with what we agreed upon
-> single version policy === exact versions
-> published package deps === caret versions
There was a problem hiding this comment.
I'm not sure this is a good idea until we have complete single version policy for dev deps (and tooling to enforce it). Instead I wonder if you could add a versionGroups entry something like this:
{
"packages": ["fluent-ui-react-repo"],
"dependencies": ["tslib", ...]
}
(If there's some other syncpack setting we need to support this scenario longer-term, the syncpack author has been pretty responsive to suggestions.)
There was a problem hiding this comment.
this works:
{
"packages": [
"fluent-ui-react-repo"
],
"dependencies": [
"tslib",
"webpack-bundle-analyzer",
"chalk"
]
},
although I don't think this is what we want, as it will allow further misalignments between versions.
What would be probably needed as a feature for syncpack is the ability to specify devDependencies similarly to how dependencies work.
| @@ -1,7 +1,7 @@ | |||
| // @ts-check | |||
| const path = require('path'); | |||
| const child_process = require('child_process'); | |||
| const chalk = require('chalk').default; | |||
| const chalk = require('chalk'); | |||
| @@ -32,6 +32,10 @@ jobs: | |||
| yarn checkchange | |||
| displayName: check change | |||
|
|
|||
| - script: | | |||
| yarn nx workspace-lint | |||
| displayName: nx:workspace-lint | |||
There was a problem hiding this comment.
run nx workspace lint checks on ci to prevent getting out of sync whilst doing changes by hand
| }, | ||
| "include": [], | ||
| "files": [], | ||
| "references": [ |
There was a problem hiding this comment.
environment scoped TS config - this is the future for every convergence package (will mitigate issues like this -> #17101)
| @@ -0,0 +1,13 @@ | |||
| { | |||
There was a problem hiding this comment.
this is only one special case for nx, as it compiles down TS source of workspace generators - it uses this file in particular
There was a problem hiding this comment.
I'm still a bit confused by intention of both tsconfig.tools.json and tsconfig.lib.json, why are both necessary ?
There was a problem hiding this comment.
sorry about that.
tsconfig.tools.jsonis used to runtscwhen executingnx workspace-generator <generator-name>runnertsconfig.lib.jsonis standard part of every package ( 100% converged to nx ), which is used for production builds (transpile/rollup) and as source of truth for type checking implementation files.- I added it to
/toolsto keep the default boilerplate consistent
- I added it to
| @@ -472,6 +468,33 @@ | |||
| "@fluentui/react-popover": { | |||
| "root": "packages/react-popover", | |||
| "projectType": "library" | |||
| }, | |||
| "nx-workspace-tools": { | |||
There was a problem hiding this comment.
this is a showcase how task managed will look like in the future with full nx adoption. for now, it's leveraged only for tools/
There was a problem hiding this comment.
Will we be duplicating this kind of json config for all packages listed in the workspace.json in the future ?
There was a problem hiding this comment.
Will we be duplicating this kind of json config for all packages listed in the workspace.json in the future ?
we might, yes -> meaning no more package.json#scripts. It gives us various benefits over primitive npm scripts, integration with vscode plugin etc.
spoiler alert -> nx 13 will add new features that this huge workspace json will be dissected into smaller ones per package
Hotell
requested review from
assuncaocharles,
ecraig12345,
jurokapsiar,
JustSlone,
layershifter,
ling1726,
miroslavstastny,
pompomon and
a team
as code owners
| { | ||
| "extends": ["plugin:@fluentui/eslint-plugin/node"], | ||
| "rules": { | ||
| "import/no-extraneous-dependencies": "off" |
There was a problem hiding this comment.
I guess anything in tools will not declare dependencies in package.json and just consume from the root right ?
There was a problem hiding this comment.
yes, single version policy from the start.
- also because this will never be published
- there is no need to provide deps
- if we might change our minds, nx generates package.json with deps automatically (based on AST/dependency graph)
| @@ -0,0 +1,10 @@ | |||
| export interface WorkspaceGeneratorGeneratorSchema { | |||
There was a problem hiding this comment.
is this generated from the schema.json or maintained manually ?
There was a problem hiding this comment.
generated. we can add that tooling later.
| throw new Error('name is required'); | ||
| } | ||
|
|
||
| const defaults = { skipFormat: false }; |
There was a problem hiding this comment.
this default value is declared in schema.json, does this mean nx does not handle default values declared in the schema ?
There was a problem hiding this comment.
this default value is declared in schema.json, does this mean nx does not handle default values declared in the schema ?
- it does, typescript doesn't - the ultimate problem of default vs optional values.
JSX has special hack for that in TS compiler (via *.defaultProps), outside of that we are out of luck. I mean it can be hacked via additional TS type transforms which adds additional churn, from my experience this is the most concise way. If you have other proposal how to handle this lemme know!
The "churn" I mentioned:
import { WorkspaceGeneratorGeneratorSchema } from './schema';
// while this looks "ok" for 1 property, for more it might become hard to read ? (YMMV)
interface SchemaWithAccommodatedDefaults extends Required<Pick<WorkspaceGeneratorGeneratorSchema,'skipFormat'>>,WorkspaceGeneratorGeneratorSchema {}
export default async function(host: Tree, schema: SchemaWithAccommodatedDefaults) {
// ....
}
package.json
Outdated
| @@ -176,7 +184,7 @@ | |||
| }, | |||
| "syncpack": { | |||
| "prod": true, | |||
| "dev": true, | |||
| "dev": false, | |||
There was a problem hiding this comment.
I'm not sure this is a good idea until we have complete single version policy for dev deps (and tooling to enforce it). Instead I wonder if you could add a versionGroups entry something like this:
{
"packages": ["fluent-ui-react-repo"],
"dependencies": ["tslib", ...]
}
(If there's some other syncpack setting we need to support this scenario longer-term, the syncpack author has been pretty responsive to suggestions.)
| { | ||
| "extends": ["plugin:@fluentui/eslint-plugin/node"], | ||
| "rules": { | ||
| "import/no-extraneous-dependencies": "off" |
There was a problem hiding this comment.
What's the reasoning for turning this off? Was the config not properly allowing deps from the root config (there's a setting for that), or some other issue?
There was a problem hiding this comment.
I know there is the setting, but it doesn't work unfortunately
There was a problem hiding this comment.
This works for me locally:
"import/no-extraneous-dependencies": ["error", { "packageDir": [".", ".."] }]
There was a problem hiding this comment.
Actually never mind, that works in the UI but not in the tool. To make it work while running yarn lint it has to be "packageDir": ["."]--I guess nx run runs stuff with cwd as the repo root?
| "compilerOptions": { | ||
| "module": "commonjs", | ||
| "target": "ES2015", | ||
| "outDir": "../dist/out-tsc", |
There was a problem hiding this comment.
I don't think we ever made a decision about whether to put the output in a single dist folder at the root. Unless it's necessary for nx to have it at the root for some reason, could it stay inside the tools folder for now?
There was a problem hiding this comment.
Unless it's necessary for nx to have it at the root for some reason, could it stay inside the tools folder for now
- right, this is default nx boilerplate. In general every build output is stored within root
dist/->/dist/<package-name>/.....- a) this is required for TS when path aliases are used ( currently we "hack it" by removing aliases during lage builds)
- b) I mentioned reasoning in this RFC
- in reality within /tools, this config file is solely for proper type checking per environment ( in this case,
lib-> implementation code )
yup, that was my plan. We might extend the meeting time because of this. |
layershifter
changed the title
|
🎉 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: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
@Hotell When I try to run (Edit: changing |
Pull request checklist
$ yarn changeDescription of changes
this PR addresses 2 things
1. bumping/resolving issues with chalk
2. adds nx workspace-tools capabilities
/tools"project" which is a default nx tooling root for authoring workspace related generators and executorsworkspace-generatorgenerator which will be used as basic building block for bootstraping workspace related generators/migrations (adds proper testing setup and schematic interface boilerplate, which is missing in default nx workspace-generator )About: Running workspace generators
yarn nx workspace-generator [generator-name]↓↓↓↓
↓↓↓↓
Focus areas to test
(optional)