Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Move dependencies/scripts from root into workspace #16640

Merged
merged 6 commits into from
Jul 19, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jul 18, 2019

Based of #16092 (azure didn't pick it up)

BREAKING

- yarn workspace docs typescript:formatted
+ yarn workspace docs typescript:transpile

This is just a refactor so don't expect immediate features. The features this will enable are:

  • focused CI/CD installs reducing install times for certain jobs (currently buggy in yarn v1)
  • reduced top level scripts with yarn v2 (current script entries in package.json will become obsolete)

Refactoring improvements:

  • reduce top level directories by 2
  • separate dependencies (whats lib dev vs whats app code?)
  • docs related "stuff" lives closer together
  • clearer distinction between lib and app transpilation (next.js technically didn't allow the current way of setting presets)

I hope that the new layout will make it easier to debug why we can't esmodules in our docs.

Test plan:

  • scripts

    • api
    • buld
    • build-sw
    • deploy
    • dev
    • export
    • icons
    • size-why
    • start
    • i18n
    • typescript
    • typescript:check
    • typescript:formatted
  • successful deploys over time

@eps1lon eps1lon mentioned this pull request Jul 18, 2019
15 tasks
"docs:dev": "rimraf node_modules/.cache/babel-loader && cross-env BABEL_ENV=docs-development node docs/src/server.js",
"docs:export": "rimraf docs/export && next export -o docs/export && yarn docs:build-sw && cp -r docs/static/. docs/export",
"docs:icons": "rimraf static/icons/* && node ./docs/scripts/buildIcons.js",
"docs:api": "rimraf ./docs/pages/api && cross-env BABEL_ENV=test babel-node ./docs/scripts/buildApi.js ./packages/material-ui/src ./docs/pages/api && cross-env BABEL_ENV=test babel-node ./docs/scripts/buildApi.js ./packages/material-ui-lab/src ./docs/pages/api",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to stay for now since they abuse the babel test environment.

@eps1lon eps1lon changed the title [docs] Move docs scripts into workspace [docs] Create separate workspace Jul 18, 2019
docs/package.json Outdated Show resolved Hide resolved
@eps1lon eps1lon changed the title [docs] Create separate workspace [docs] Move dependencies/scripts from root into workspace Jul 18, 2019
@eps1lon eps1lon force-pushed the docs/workspace-azure branch from 45217d5 to 4944bb7 Compare July 18, 2019 15:23
@mui-pr-bot
Copy link

mui-pr-bot commented Jul 18, 2019

Details of bundle changes.

Comparing: b3218e7...6366654

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 327,251 327,251 90,345 90,345
@material-ui/core/Paper 0.00% 0.00% 68,477 68,477 20,410 20,410
@material-ui/core/Paper.esm 0.00% 0.00% 61,761 61,761 19,177 19,177
@material-ui/core/Popper 0.00% 0.00% 28,896 28,896 10,394 10,394
@material-ui/core/Textarea 0.00% 0.00% 5,534 5,534 2,369 2,369
@material-ui/core/TrapFocus 0.00% 0.00% 3,753 3,753 1,577 1,577
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,156 16,156 5,816 5,816
@material-ui/core/useMediaQuery 0.00% 0.00% 3,098 3,098 1,310 1,310
@material-ui/lab 0.00% 0.00% 141,699 141,699 43,813 43,813
@material-ui/styles 0.00% 0.00% 51,886 51,886 15,380 15,380
@material-ui/system 0.00% 0.00% 15,576 15,576 4,445 4,445
Button 0.00% 0.00% 79,711 79,711 24,358 24,358
Modal 0.00% 0.00% 14,548 14,548 5,102 5,102
Portal 0.00% 0.00% 3,471 3,471 1,568 1,568
Rating 0.00% 0.00% 70,267 70,267 22,068 22,068
Slider 0.00% 0.00% 75,096 75,096 23,311 23,311
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing +0.03% 🔺 +1.11% 🔺 54,338 54,357 13,762 13,915
docs.main -0.15% -0.63% 647,768 646,774 204,322 203,034
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 299,666 299,666 86,112 86,112

Generated by 🚫 dangerJS against 6366654

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Jul 18, 2019
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done :). I'm really happy to see the documentation pages centralized.

I have a nip note regarding this folder: /docs/netlify.
Half of the files in this folder are not directly related to Netlify. What do you think of renaming the folder /docs/cdn?

reduced top level scripts with yarn v2 (current script entries in package.json will become obsolete)

Does it refer to #15497? +1 for consolidating all the scripts under a single place, a single place that people can discover, browse and learn more about.
I often find myself running npm run to see what the available command are on a project (even with Material-UI because I have poor memory retention). I'm not aware of a similar command with yarn. I wish these CI tools embraced the scripts documentation problem natively (it's the problem we are trying to solve, right?).

package.json Outdated Show resolved Hide resolved
docs/next.config.js Outdated Show resolved Hide resolved
docs/package.json Show resolved Hide resolved
@eps1lon
Copy link
Member Author

eps1lon commented Jul 19, 2019

Half of the files in this folder are not directly related to Netlify. What do you think of renaming the folder /docs/cdn

Sounds fine. Was just the first thing to cross my mind.

Do we need the favicon duplication? Currently we have one under static/favicon and docs/static/favicon.

I'm not aware of a similar command with yarn.

yarn run should work the same. I'll need to checkout how this works in v2. I think there were improvements planned for this as well.

I often find myself running npm run to see what the available command are on a project (even with Material-UI because I have poor memory retention).

Yeah this is more of a concern for #15497. The scripts format just doesn't scale beyond 20 or 30 scripts and multiple workspaces.

But at least you have yarn run for the root (scripts concerning the repo) and yarn workspace @material-ui/core run for a single workpspace (scripts concerning a package). Maybe we get workspace aliases in yarn v2 @material-ui/ is pretty annoying to type.

@eps1lon eps1lon force-pushed the docs/workspace-azure branch from 4944bb7 to 7e6fac3 Compare July 19, 2019 10:11
@mbrookes
Copy link
Member

We could add a root script for it 😄:

"core:run": "yarn workspace @material-ui/core run",

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 19, 2019

Do we need the favicon duplication? Currently we have one under static/favicon and docs/static/favicon.

@eps1lon We need the favicon at the root level for production. We need the favicon at the /static folder to remove 404 errors in development mode (browsers aggressive favicon fetching). This is a quick and dirty fix. I think that a proper solution would be to keep the favicon at the cdn folder but to add a static middleware in our the development express server to serve the files in the /cdn folder. Does it worth the effort?

diff --git a/pages/_document.js b/pages/_document.js
index 76beb8ee0..bfa1cd263 100644
--- a/pages/_document.js
+++ b/pages/_document.js
@@ -49,7 +49,7 @@ class MyDocument extends Document {
           <link rel="manifest" href="/static/manifest.json" />
           {/* PWA primary color */}
           <meta name="theme-color" content={themeColor} />
-          <link rel="shortcut icon" href="/static/favicon.ico" />
+          <link rel="shortcut icon" href="/favicon.ico" />
           {/* SEO */}
           <link
             rel="canonical"

yarn run should work the same.

Oh yes, it does, at least with yarn v1.17.0 😍.

The scripts format just doesn't scale beyond 20 or 30 scripts and multiple workspaces.

I'm curious to understand what you mean by it doesn't scale :)

@eps1lon
Copy link
Member Author

eps1lon commented Jul 19, 2019

I'm curious to understand what you mean by it doesn't scale :)

For a couple of scripts it's usually pretty obvious what their purpose is or what they do (build, test, lint, format etc). Once the workspace grows (in size and usage) more esoteric scripts get added. Explaining every script requires an additional section in CONTRIBUTING that can easily become out of date. So with more scripts documentation gets worse which can't be inlined into package.json.

The other issue is that you need to consider cross environment concerns. It's easy to be lazy and put in the occasional cd or piping operator. If scripts are required to be node.js scripts cross environment compatibility is more likely to be given.

And then there's readability of a script. Add 2 env variables, 3 cli flags and maybe pipe the exit code and readability as well as deuggability is usually gone at that point.

@eps1lon eps1lon force-pushed the docs/workspace-azure branch from 7e6fac3 to cd8cba8 Compare July 19, 2019 18:36
@eps1lon eps1lon force-pushed the docs/workspace-azure branch from cd8cba8 to 105afb8 Compare July 19, 2019 18:47
@eps1lon
Copy link
Member Author

eps1lon commented Jul 19, 2019

We need the favicon at the root level for production. We need the favicon at the /static folder to remove 404 errors in development mode (browsers aggressive favicon fetching). This is a quick and dirty fix.

Nah that's fine. This almost never changes and we never had inconsistent icons. Just wanted to make sure this actually serves a purpose.

Fine by me for merging. Any concerns remaining?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 19, 2019

Any concerns remaining?

Can we edit the sources and see the changes live in the dev documentation? I can't make HMR work on my env with the core components. Otherwise, it's all 👍 for me.


I think that I would personally slightly prefer having 100 scripts in the root package.json than 20 spread among 5 different files. That's my bias toward preferring monoliths. But I'm happy to try it the other way :).

@eps1lon
Copy link
Member Author

eps1lon commented Jul 19, 2019

I don't know. I would personally slightly prefer having 100 scripts in the root package.json than 20 spread among 5 different files. But I'm happy to try it the other way :).

Oh come on now. Why do you need workspaces then. We're again at a point where you argue for arguments sake.

@eps1lon
Copy link
Member Author

eps1lon commented Jul 19, 2019

Can we edit the source and see the changes live in the dev documentation? I can't make HMR work on my env with the core components. Otherwise, it's all 👍 for me.

Will look into it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 19, 2019

Oh come on now. Why do you need workspaces then. We're again at a point where you argue for arguments sake.

I "knew" I was going to trigger this response :). Anytime I see an important point where our vision might not align or an important point where I feel we haven't explored the alternatives, I will dive into it. I will try to push the exploration of the pros and cons forward. It's mostly driven by curiosity. Why would somebody else think differently? Why could have I missed? It's definitely not the first time nor the last time I do it. Don't see it as an ego war, a.k.a., who is right, but see it as a best option exploration.

I wish we didn't need workspaces in the first place. But unfortunately, we had to introduce them to solve npm code delivery issues. It probably started with the icon package, a big package, slow to download that we rarely release. And it grew from there, lab, system, styles, utils, and types.

Correct me if you think otherwise, I think that we should aim for 1. a monolith, so we can minimize the degrees of freedoms (at the cost of marginal suboptimizations). 2. and that we should aim at maximizing code removal potential (isolation). I think that the two can happen together in most cases.

End of the ().

@eps1lon eps1lon force-pushed the docs/workspace-azure branch from b36cc7b to 6366654 Compare July 19, 2019 20:59
@eps1lon eps1lon merged commit b6182ce into mui:master Jul 19, 2019
@eps1lon eps1lon deleted the docs/workspace-azure branch July 19, 2019 21:16
@oliviertassinari
Copy link
Member

Well done 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants