build(workspaces): implement src as workspace package by nickofthyme · Pull Request #1178 · elastic/elastic-charts

nickofthyme · 2021-05-30T19:41:20Z

Summary

Implement @elastic/charts/src as yarn workspace. Benefits of this are mostly internal but could improve productivity. These changes will have no effect on the library consumers.

Details

Step towards closing #378. Ensured all npm scripts function as before, some even better 😏 🎉

List of benefits

Cleaner library package.json.
Greater control over app and library build process
Cleaner dependencies in bundled library or libraries in the future
Separate dependencies for app and library, isolation between apps and library (allows for package.json "engine" is too restrictive #359)
Applications can use built library instead of src files.
Dependencies can be linked together, which means that your workspaces can depend on one another while always using the most up-to-date code available.
All your project dependencies will be installed together, giving Yarn more latitude to better optimize them.
Yarn will use a single lockfile rather than a different one for each project, which means fewer conflicts and easier reviews.

List of key changes

move src/** to packages/elastic-charts/**
correct all paths due to file structure changes
fix bad storybook-docs paths and config options
create top-level mono package, move lib package
simplify lib tsconfig.json files
fix errors in link_kibana package, cleanup path logic
fix linting config to reflect file structure changes
update semantic release to point at new lib package
add lerna and @lerna/run to facilitate running workspace/package scripts

File Structure

# before
elastic-charts/
├── src/
└── package.json

# after
elastic-charts/
├── packages/
│   ├── elastic-charts/
│   │   ├── src/
│   │   └── package.json
└── package.json

As you may have noticed, there are now two package.json files. The main top-level package.json (named elastic-charts-mono) is needed to control the repo and all its apps and libraries via workspaces. The other one at packages/elastic-charts/package.json is the package for the library itself, moved from the top-level.

One of the beauties of this new approach is offloading all of the devDependencies to the top level package.json that can be used across all workspaces.

A more complete example of the workspace file structure is react, see https://github.com/facebook/react

Limitations

Currently semantic release assumes a 1:1 relation between repo and packages (see semantic-release/semantic-release#193). This is fine for now with only one external package, but this could be addressed when we have more packages to release.

Checklist

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios

- move `./src/**` to `./packages/elastic-charts/**` - correct all paths due to file sturcture changes - fix bad docs paths and config options - create top-level mono package, move lib package - simplify lib tsconfig.json files - fix errors in linking package, cleanup paths - fix linting config to reflect file structure changes - update semantic release to point at new lib package - add lerna to facilitate running workspace scripts

nickofthyme

A few code comments to explain some of the key changes.

nickofthyme · 2021-05-30T23:00:53Z

packages/elastic-charts/src/reset_dark.scss

+@import '../../../node_modules/@elastic/eui/src/themes/eui/eui_colors_dark';
+@import 'style/themes/colors_dark';
+@import '../../../node_modules/@elastic/eui/src/global_styling/functions/index';
+@import '../../../node_modules/@elastic/eui/src/global_styling/variables/index';
+@import '../../../node_modules/@elastic/eui/src/global_styling/mixins/index';
+@import '../../../node_modules/@elastic/eui/src/global_styling/reset/index';


git could not track this rename, but this is the same file as src/reset_dark.scss. Only the path changed to point at top node_modules folder.

nickofthyme · 2021-05-30T23:05:04Z

packages/link_kibana/replace_modules.js

+  const cwd = path.join(libDir, 'dist');
+  const nodeModulesDir = path.join(libDir, 'node_modules');
+  const re = new RegExp(`require\\("(?:.*?\\/)*(${linkedPackages.join('|')})"\\)`, 'g');


Now this simply converts to relative path require('react') -> require('../react'). Previously went into library like require('../../kibana/node_modules/react')

Now when linked elsewhere the dist files will use the relative node_modules depending on where the package is linked.

nickofthyme · 2021-05-30T23:05:50Z

src/_eui_imports.scss

-@import '../node_modules/@elastic/eui/src/global_styling/functions/index';
-@import '../node_modules/@elastic/eui/src/global_styling/variables/index';
-@import '../node_modules/@elastic/eui/src/global_styling/mixins/index';


Not delete only moved to packages/elastic-charts/src

nickofthyme · 2021-05-30T23:06:37Z

tslint.yaml

-extends:
-  - tslint:recommended
-  # - tslint-microsoft-contrib
-
-rulesDirectory:
-  # - node_modules/tslint-microsoft-contrib
-
-rules:
-  object-literal-sort-keys: false
-  interface-name: false
-  no-default-export: true
-  no-irregular-whitespace: true
-  no-unused-expression: true
-  quotemark: [true, 'single', 'jsx-double']
-  member-access: [true, 'no-public']


We have not used tslint since I can recall

nickofthyme · 2021-05-30T23:07:22Z

tsconfig.json

@@ -21,5 +20,5 @@
    "resolveJsonModule": true,
    "typeRoots": ["./node_modules/@types", "./**/*.d.ts", "./scripts/custom_matchers.ts"]
  },
-  "exclude": ["**/tmp"]
+  "exclude": ["**/tmp", "**/dist"]


prevents IDE from attempting to auto import from dist directory

isn't that excluded with .gitignore?

I kept getting this dist import suggestions, maybe but I think dist was already gitignored

nickofthyme · 2021-05-30T23:18:16Z

package.json

  "scripts": {
-    "autoprefix:css": "echo 'Autoprefixing...' && yarn postcss dist/*.css --no-map --use autoprefixer -d dist",
-    "api:check": "yarn build:ts && yarn api:extract",
-    "api:check:local": "yarn api:check --local",
-    "api:extract": "yarn api-extractor run --verbose",
+    "autoprefix:css": "lerna run --loglevel=silent --scope @elastic/charts autoprefix:css",
+    "api:check": "lerna run --loglevel=silent --scope @elastic/charts api:check",
+    "api:check:local": "lerna run --loglevel=silent --scope @elastic/charts api:check:local",
+    "api:extract": "lerna run --loglevel=silent --scope @elastic/charts api:extract",


Kept all previous scripts, except for watch which I never used as it's faster to just saw yarn test -w.

Now repo-wide scripts are run from here and library scripts are run using @lerna/run

I think we can cleanup these scripts more in the future or even have a cli much like yarn kbn <action>

nickofthyme · 2021-05-30T23:18:47Z

package.json

-  "module": "dist/index.js",
-  "types": "dist/index.d.ts",
+  "private": "true",
  "repository": "git@github.com:elastic/elastic-charts.git",
-  "publishConfig": {
-    "access": "public"
-  },
-  "files": [
-    "dist/**/*"
+  "workspaces": [
+    "packages/*"


must be private to add workspaces

nickofthyme · 2021-05-30T23:19:12Z

package.json

+  "name": "elastic-charts-mono",
+  "description": "Mono repo for @elastic/charts",


Open to a better name or description her just cannot be @elastic/charts

nickofthyme · 2021-05-30T23:20:27Z

.releaserc.js

+    ...(isDryRun
+      ? []
+      : [
+          'semantic-release-slack-bot',
+          {
+            notifyOnSuccess: true,
+            notifyOnFail: true,
+            markdownReleaseNotes: true,
+          },
+        ]),


This always fails when running in dry-run mode

nickofthyme · 2021-05-30T23:23:28Z

.releaserc.js

+    [
+      '@semantic-release/npm',
+      {
+        // must point to the child package
+        pkgRoot: 'packages/elastic-charts',
+      },
+    ],


I'm pretty sure this will work and is the only change needed to point to the now nested library. 🤞🏼 See @semantic-release/npm

If this does not work, we can move this config to packages/elastic-charts.

We could update the git and github assets to only include the dist but I didn't see other orgs like react doing this on github.

monfera · 2021-05-31T08:51:52Z

What's the reason for putting src into somewhere deep? Also I'm not sure how I feel about elastic-charts/packages/elastic-charts/src/ or similar paths with duplicated directory names. The outer elastic-charts refers to the containing directory, though its name is automatically determined by cloning the repo.

If I understand, elastic-charts is a single thing, and this file structuring change feels like we're turning it into some monorepo, what's the reason for this? Also, in this case, shouldn't the outer or inner elastic-charts be named differently? Eg. elastic-charts/packages/charts/src/, with the idea that maybe some of the logic that isn't charts dependent moves into eg. elastic-charts/packages/charts-tooltip/src/ (assuming that our tooltip is made available to other users, without them having to use the entire charts library)

I'll put it on today's agenda to learn more

markov00 · 2021-05-31T10:13:58Z

If I understand, elastic-charts is a single thing, and this file structuring change feels like we're turning it into some monorepo, what's the reason for this?

@monfera most of the benefits are described clearly in the PR description and in the 2 linked issues and can be resumed in a cleaner set of dependencies for the library and a clear distinction between library and the docs+storybook infrastructure.

This monorepo can be exploited in the future to exposing multiple independent packages within the same repo. One example could be: split the React Chart API from the Chart implementation and rendering whenever we have defined a clean JSON, JSON like API, so that one can just import the chart implementation and use the JSON API, or someone wants to use the react API bridge to build charts.

You are probably right about the folder naming: calling them charts is probably way better, and also reflect a bit more the current npm package name: @elastic/charts

monfera · 2021-05-31T11:00:55Z

Thanks @markov00!

This monorepo can be exploited in the future to exposing multiple independent packages within the same repo

That's great, if it is a monorepo indeed. We've been talking about a decomposition anyway, so at some point, charts proper might only represent the (equivalent of the) current API. Would be possible to add other builds too, eg. Cartesian-only etc., or share a common time axis logic and/or axis rendering with other teams, as a random example 😄

nickofthyme · 2021-05-31T13:00:17Z

What's the reason for putting src into somewhere deep? Also I'm not sure how I feel about elastic-charts/packages/elastic-charts/src/ or similar paths with duplicated directory names. The outer elastic-charts refers to the containing directory, though its name is automatically determined by cloning the repo.

@monfera You are right It is a little deeper but the idea is that we have now the ability to have independent packages that are either coupled with @elastic/charts or their own independent package.

But the beauty about yarn workspaces is that they automatically symlink all dependent packages across the repo. Thus if I am in stories/* or any other top-level directory I can simply import from @elastic/charts instead of ../../src/*. I have not done this yet just to focus on moving the charts library into packages with minimal code diff.

If I understand, elastic-charts is a single thing, and this file structuring change feels like we're turning it into some monorepo, what's the reason for this? Also, in this case, shouldn't the outer or inner elastic-charts be named differently? Eg. elastic-charts/packages/charts/src/, with the idea that maybe some of the logic that isn't charts dependent moves into eg. elastic-charts/packages/charts-tooltip/src/ (assuming that our tooltip is made available to other users, without them having to use the entire charts library)

Yes it is a single thing, at least for the moment, but this not only allows for adding more packages in the future but also clearly defining boundaries of the library and the other apps. This includes moving all our application code such as stories/, storybook[-docs]/, playground/ into their own isolated workspace.

This monorepo can be exploited in the future to exposing multiple independent packages within the same repo. One example could be: split the React Chart API from the Chart implementation and rendering whenever we have defined a clean JSON, JSON like API, so that one can just import the chart implementation and use the JSON API, or someone wants to use the react API bridge to build charts.

Exactly! Similarly, we could even have separate rending packages for canvas, svg and WebGL.

markov00 · 2021-05-31T14:02:08Z

Thus if I am in stories/* or any other top-level directory I can simply import from @elastic/charts instead of ../../src/*.

@nickofthyme this is a great thing, it helps a lot with the testing as we isolate the lib from Storybook and help us identifying missing exposed types easily.

markov00 · 2021-06-07T08:41:14Z

@nickofthyme I've pull down this PR and run yarn to update the packages but got this error:

 yarn
yarn install v1.22.5
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
[4/5] 🔗  Linking dependencies...
warning " > @elastic/eui@32.1.0" has unmet peer dependency "@types/react@^16.9.34".
warning "@storybook/addon-actions > @storybook/api@5.3.21" has unmet peer dependency "regenerator-runtime@*".
warning "@storybook/addon-docs > @egoist/vue-to-react@1.1.0" has unmet peer dependency "vue@^2.6.10".
warning "@storybook/addon-docs > vue-docgen-api > @vue/compiler-sfc@3.0.0-rc.10" has unmet peer dependency "vue@3.0.0-rc.10".
[5/5] 🔨  Building fresh packages...
$ echo 'This package os not published, see pacakges/*' && exit 1
This package os not published, see pacakges/*
error Command failed with exit code 1.

Why is the prepublish script called here?

nickofthyme · 2021-06-07T14:16:55Z

@nickofthyme I've pull down this PR and run yarn to update the packages but got this error:

 yarn
yarn install v1.22.5
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
[4/5] 🔗  Linking dependencies...
warning " > @elastic/eui@32.1.0" has unmet peer dependency "@types/react@^16.9.34".
warning "@storybook/addon-actions > @storybook/api@5.3.21" has unmet peer dependency "regenerator-runtime@*".
warning "@storybook/addon-docs > @egoist/vue-to-react@1.1.0" has unmet peer dependency "vue@^2.6.10".
warning "@storybook/addon-docs > vue-docgen-api > @vue/compiler-sfc@3.0.0-rc.10" has unmet peer dependency "vue@3.0.0-rc.10".
[5/5] 🔨  Building fresh packages...
$ echo 'This package os not published, see pacakges/*' && exit 1
This package os not published, see pacakges/*
error Command failed with exit code 1.

Why is the prepublish script called here?

Interesting, might be related to yarnpkg/yarn#3209. I'll use prepack for now see 2761568. The idea is to block that package from being published.

Key changes: - move `./src/**` to `./packages/elastic-charts/**` - correct all paths due to file sturcture changes - fix bad docs paths and config options - create top-level mono package, move lib package - simplify lib tsconfig.json files - fix errors in linking package, cleanup paths - fix linting config to reflect file structure changes - update semantic release to point at new lib package - add lerna to facilitate running workspace scripts

# [30.1.0-alpha.1](v30.0.0...v30.1.0-alpha.1) (2021-06-08) ### Features * **workspaces:** implement src as workspace package ([#1178](#1178)) ([0cdf3b4](0cdf3b4))

Key changes: - move `./src/**` to `./packages/elastic-charts/**` - correct all paths due to file sturcture changes - fix bad docs paths and config options - create top-level mono package, move lib package - simplify lib tsconfig.json files - fix errors in linking package, cleanup paths - fix linting config to reflect file structure changes - update semantic release to point at new lib package - add lerna to facilitate running workspace scripts

nickofthyme added dependencies Pull requests that update a dependency file :ci :build Build tools / dependencies labels May 30, 2021

nickofthyme requested review from markov00, monfera and rshen91 May 30, 2021 19:41

nickofthyme mentioned this pull request May 30, 2021

Convert to yarn workspaces #378

Open

6 tasks

nickofthyme linked an issue May 30, 2021 that may be closed by this pull request

Convert to yarn workspaces #378

Open

6 tasks

nickofthyme added 2 commits May 30, 2021 15:04

test: fix test coverage patterns

445b0f7

chore: revert auto fix changes to d3-delaunay

64dcf09

nickofthyme commented May 30, 2021

View reviewed changes

nickofthyme added 2 commits June 5, 2021 20:41

chore: update package author and prepublish

cf9dd62

Merge remote-tracking branch 'upstream/master' into lib-workspace

ecc61dd

Merge branch 'master' into lib-workspace

0b9e26c

nickofthyme added 4 commits June 7, 2021 09:17

chore: fix prepublish yarn issue

2761568

refactor: rename package from elastic-charts to charts

ed83b52

Merge branch 'master' into lib-workspace

520c917

Merge branch 'master' into lib-workspace

b689791

nickofthyme changed the base branch from master to alpha June 8, 2021 16:19

nickofthyme merged this pull request into elastic:alpha Jun 8, 2021

nickofthyme deleted the lib-workspace branch June 8, 2021 16:21

nickofthyme mentioned this pull request Jun 8, 2021

build(workspaces): implement src as workspace package (#1178) [master] #1197

Closed

nickofthyme pushed a commit that referenced this pull request Jun 8, 2021

chore(release): 30.1.0-alpha.1 [skip ci]

b797495

# [30.1.0-alpha.1](v30.0.0...v30.1.0-alpha.1) (2021-06-08) ### Features * **workspaces:** implement src as workspace package ([#1178](#1178)) ([0cdf3b4](0cdf3b4))

nickofthyme mentioned this pull request Jun 8, 2021

build(workspaces): implement src as workspace package (#1178) #1198

Merged

nickofthyme mentioned this pull request Sep 20, 2021

docs(playground): fix webpack config #1391

Merged

		"name": "elastic-charts-mono",
		"description": "Mono repo for @elastic/charts",

Conversation

nickofthyme commented May 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

List of benefits

List of key changes

File Structure

Limitations

Checklist

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickofthyme May 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

nickofthyme May 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfera commented May 31, 2021

Uh oh!

markov00 commented May 31, 2021

Uh oh!

monfera commented May 31, 2021

Uh oh!

nickofthyme commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markov00 commented May 31, 2021

Uh oh!

markov00 commented Jun 7, 2021

Uh oh!

nickofthyme commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nickofthyme commented May 30, 2021 •

edited

Loading

nickofthyme May 30, 2021 •

edited

Loading

nickofthyme May 30, 2021 •

edited

Loading

nickofthyme commented May 31, 2021 •

edited

Loading

nickofthyme commented Jun 7, 2021 •

edited

Loading