Initial frontend monorepo structure #1362

vojtechszocs · 2019-03-29T18:06:37Z

Frontend monorepo

This PR does two things:

address RFE Alternative way to manage Yarn installation #1363 for easier Yarn version management
add initial monorepo structure while keeping the public directory intact

Closes https://jira.coreos.com/browse/CONSOLE-1241
Closes #1363

Note: any file paths mentioned below are meant in context of the frontend directory.

Yarn version management

.yarnrc is used to delegate execution to the local binary at .yarn/releases/yarn-<version>.js via the yarn-path option. This option is supported by Yarn >= 1.0 so there's no need to bump the global version installed on the system.

This causes the local Yarn binary to be used by both developers and automated systems such as CI/CD. Switching to a newer version of Yarn becomes super easy - put the new binary in .yarn/releases and update .yarnrc accordingly.

Developers can override this behavior by setting yarn-path to false if necessary.

Monorepo support

The monorepo is powered by Yarn workspaces, a feature introduced in Yarn v0.28 (Jul 2017) and enabled by default since Yarn v1.0.0 (Sep 2017).

Yarn’s workspaces are the low-level primitives that tools like Lerna can (and do!) use. They will never try to support the high-level feature that Lerna offers, but by implementing the core logic of the resolution and linking steps inside Yarn itself we hope to enable new usages and improve performance.

This PR avoids using Lerna since Yarn workspaces already covers all of our monorepo requirements:

define packages using the package.json standard
link packages using the node_modules file structure ⭐
run a script on all the packages (Yarn v1.10 or better)

⭐ Hoisting of dependencies (moving them "up" into root node_modules directory) is an optimization to eliminate redundancy and improve performance. Yarn hoists by default but also supports the nohoist option to circumvent potential issues. See also Lerna docs on hoisting.

Packages

The packages directory is a flat list of all the frontend packages. The console prefix implies a core package owned by the Console team. This PR introduces two core packages:

packages/
|-- console-app
`-- console-shared

@console/app represents the Console web application itself.

for now, src/index.ts module simply imports the React application code from public directory
Console webpack entry is now defined as ['./polyfills.js', '@console/app']

@console/shared represents the code shared between all packages.

for now, src/index.ts module exports an empty object (placeholder for future updates)
this package provides its own test script to scope Jest execution (test co-location preference)

Dependencies

Existing core dependencies and devDependencies are kept in the monorepo root package.json. In general, having all external dependencies declared in one place makes it easier to maintain a coherent technology stack across all the packages and reduce potential of conflicts or duplicities.

Non-core packages can still declare specific dependencies, e.g. react-cosmos or @patternfly/react-console for a potential kubevirt-components package.

Other changes

all packages are currently private and use a fixed version (0.0.0-fixed)
packages/.eslintrc is a symlink to public/.eslintrc

In future, we'll also need to ensure that ESLint import/no-extraneous-dependencies is applied to all the packages.

/cc @spadgett @jhadvig @christianvogt @priley86 @mareklibra @rawagner @jelkosz

vojtechszocs · 2019-03-30T01:12:19Z

Created #1363 to improve on Yarn version management.

priley86 · 2019-04-01T17:29:30Z

frontend/packages/console-shared/package.json

+  "private": true,
+  "main": "src/index.ts",
+  "scripts": {
+    "test": "yarn --cwd ../.. run test packages/console-shared"


this PR is fantastic work @vojtechszocs ! ⭐️

My only feedback at this point is just merely a question...any chance the jest projects array (ProjectConfig) would be easier for you on this piece? It seems that Jest now internally supports monorepos as well.

https://jestjs.io/docs/en/configuration#projects-array-string-projectconfig

Thank you, Patrick 😃

This PR retains the existing test script (and others) in the monorepo root package.json without any changes. Those scripts represent a single execution of some tool (e.g. Jest) to operate on the whole code base.

So instead of e.g. yarn workspaces run test (analogy to lerna run test), which delegates the responsibility to each package and results in multiple Jest executions, the monorepo root's test script simply runs Jest on all the code.

In specific packages, like console-shared above, we "override" the root test script by scoping Jest execution to that package. This is to support developer experience on the package level, where you cd frontend/packages/console-shared and do yarn test etc.

Now regarding your question, I like the idea of using Jest projects feature 👍 if we can support above mentioned root & per-package developer experience. Enumerating individual packages as Jest "projects" isn't good as it imposes assumptions about concrete packages, so we could go with:

{ "projects": ["<rootDir>/packages/*"] }

but I think we'd still end up cwd-ing to monorepo root since the Jest configuration is still global:

yarn --cwd ../.. run test --projects packages/console-shared

which isn't too different from the current approach. However, I can be wrong here so let me know what you think.

The long-term idea, inspired by fabric8-ui monorepo, is to extract Jest, ESLint and other tool specific configuration & scripts into a separate package (say console-scripts) and let each package to run those scripts while reusing the same common configuration.

So in the long-term, the root test script would become yarn workspaces run test.

ok, thanks for the explanation @vojtechszocs - this syntax was a little odd to me, but now I understand. Agree that long-term yarn workspaces run test is much easier to follow and it seems this is intended ;-)
https://yarnpkg.com/lang/en/docs/cli/workspaces/#toc-yarn-workspaces-run

vojtechszocs · 2019-04-03T17:20:37Z

I'm going to update this PR to retain the public directory (should reduce the amount of changes).

vojtechszocs · 2019-04-05T17:54:30Z

/retest

spadgett · 2019-04-05T18:02:02Z

/cc @christianvogt

spadgett

Thanks @vojtechszocs. This is a great first step. I'd like to get @christianvogt's input as well.

/approve

spadgett · 2019-04-05T18:03:57Z

frontend/.yarnrc

@@ -0,0 +1 @@
+yarn-path ".yarn/releases/yarn-1.15.2.js"


I'd expect yarn.lock changes to add integrity checks, but maybe those slipped into master by mistake.

It seems that existing yarn.lock entries are not automatically updated to the latest format upon yarn install, regardless of using --update-checksums parameter (tested with Yarn 1.15.2).

However, when you add or modify a dependency, that dependency (as well as any related ones) will have their corresponding yarn.lock entries updated to the latest format.

Only a few entries in the current yarn.lock already have an integrity field. This indicates that those dependencies were added or modified using a newer version of Yarn and passed code review (people oftentimes skip over yarn.lock changes).

vojtechszocs · 2019-04-05T18:46:29Z

clean-frontend.sh

@@ -1,4 +1,4 @@
 #!/usr/bin/env bash

-rm -rf frontend/node_modules
+find frontend -name 'node_modules' -type d -delete


This will fail for non-empty directories, my mistake. I'll fix it shortly.

Fixed in the latest PR update.

christianvogt · 2019-04-05T18:15:53Z

frontend/package.json

@@ -51,7 +52,9 @@
    ],
    "collectCoverageFrom": [
      "public/*.{js,jsx,ts,tsx}",
-      "public/{components,module,ui}/**/*.{js,jsx,ts,tsx}"
+      "public/{components,module,ui}/**/*.{js,jsx,ts,tsx}",
+      "packages/**/*.{js,jsx,ts,tsx}",


We should probably only include files within the src directory of the packages:
"packages/*/src/**/*.{js,jsx,ts,tsx}",

Agreed & fixed in the latest PR update.

spadgett · 2019-04-05T19:48:25Z

We might need to rebase master-next for #1383 and #1386

spadgett · 2019-04-07T15:10:21Z

Needs #1375 to fix the deploy image test failure that appeared after the k8s rebase. I opened #1394 to fast-forward master-next.

spadgett · 2019-04-07T17:16:39Z

/retest

spadgett · 2019-04-08T00:13:24Z

/skip

spadgett · 2019-04-08T00:13:38Z

/test e2e-aws-console

christianvogt · 2019-04-08T20:47:13Z

Thanks @vojtechszocs for starting this. I know you wanted to move the code immediately into the sub-packages but I think it's better to do this in smaller steps.
Devconsole will want to create package as soon as we can get this in.

/approve

spadgett · 2019-04-08T22:48:01Z

/lgtm

openshift-ci-robot · 2019-04-08T22:48:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, spadgett, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2019-04-08T23:05:09Z

@vojtechszocs: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-console-olm	`7977ef6`	link	`/test e2e-aws-console-olm`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

spadgett · 2019-04-09T03:03:23Z

/retest

vojtechszocs · 2019-04-09T16:23:05Z

Thanks @spadgett for fast-forwarding master-next to pick up the recent fixes and merging this PR! 🎉

I know you wanted to move the code immediately into the sub-packages but I think it's better to do this in smaller steps.

Agreed. 👍 I've actually tried to symlink packages/console-app/src ⬅️ public but ran into TypeScript Duplicate identifier errors which is a known issue so I've ended up with console-app's main script just importing stuff from public.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 29, 2019

openshift-ci-robot requested review from dtaylor113 and jhadvig March 29, 2019 18:07

vojtechszocs marked this pull request as ready for review March 29, 2019 20:43

priley86 reviewed Apr 1, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2019

Fix RFE openshift#1363

37b809f

vojtechszocs force-pushed the monorepo-restructure branch from 3f9803a to acbe9b0 Compare April 5, 2019 15:41

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 5, 2019

vojtechszocs changed the title ~~WIP frontend monorepo restructure~~ Initial frontend monorepo structure Apr 5, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2019

openshift-ci-robot requested a review from christianvogt April 5, 2019 18:02

spadgett approved these changes Apr 5, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2019

vojtechszocs commented Apr 5, 2019

View reviewed changes

christianvogt reviewed Apr 5, 2019

View reviewed changes

vojtechszocs force-pushed the monorepo-restructure branch from acbe9b0 to afe6d70 Compare April 5, 2019 19:04

Initial monorepo structure

7977ef6

vojtechszocs force-pushed the monorepo-restructure branch from afe6d70 to 7977ef6 Compare April 5, 2019 19:10

spadgett added this to the v4.2 milestone Apr 5, 2019

christianvogt approved these changes Apr 8, 2019

View reviewed changes

openshift-ci-robot assigned spadgett Apr 8, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2019

openshift-merge-robot merged commit 17abae4 into openshift:master-next Apr 9, 2019

vojtechszocs mentioned this pull request Apr 9, 2019

Alternative way to manage Yarn installation #1363

Closed

vojtechszocs deleted the monorepo-restructure branch April 9, 2019 16:23

vojtechszocs mentioned this pull request Apr 9, 2019

KubeVirt extension PoC #888

Closed

3 tasks

Initial frontend monorepo structure #1362

Initial frontend monorepo structure #1362

Uh oh!

Conversation

vojtechszocs commented Mar 29, 2019 • edited by spadgett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Frontend monorepo

Yarn version management

Monorepo support

Packages

Dependencies

Other changes

Uh oh!

vojtechszocs commented Mar 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vojtechszocs commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vojtechszocs commented Apr 5, 2019

Uh oh!

spadgett commented Apr 5, 2019

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Apr 5, 2019

Uh oh!

spadgett commented Apr 7, 2019

Uh oh!

spadgett commented Apr 7, 2019

Uh oh!

spadgett commented Apr 8, 2019

Uh oh!

spadgett commented Apr 8, 2019

Uh oh!

christianvogt commented Apr 8, 2019

Uh oh!

spadgett commented Apr 8, 2019

Uh oh!

openshift-ci-robot commented Apr 8, 2019

Uh oh!

openshift-ci-robot commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett commented Apr 9, 2019

Uh oh!

vojtechszocs commented Apr 9, 2019

Uh oh!

Uh oh!

vojtechszocs commented Mar 29, 2019 •

edited by spadgett

Loading

vojtechszocs commented Apr 3, 2019 •

edited

Loading

openshift-ci-robot commented Apr 8, 2019 •

edited

Loading