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

Reduce churn #1015

Closed
wooorm opened this issue Apr 14, 2020 · 4 comments
Closed

Reduce churn #1015

wooorm opened this issue Apr 14, 2020 · 4 comments
Labels
👩‍⚕ area/health This affects community 🏡 area/internal This affects the hidden internals 💬 type/discussion This is a request for comments

Comments

@wooorm
Copy link
Member

wooorm commented Apr 14, 2020

Hi folks! 👋

I just came back to this repo after a while, going through issues, PRs, dependency updates, etc, and this may just be me but to me it feels like there’s a bit much churn to maintain this repo:

  • Building is long: takes about 5 to 8 minutes for a CI job to finish; not too important, but it takes 2:30 to install dependencies, between 2 and 4 minutes to store a cache version of them, …
  • GH CI sometimes randomly fails: for a PR pushed from the repo, there are 18 CI builds done 2 (PR + Push) * 3 (macos, windows, ubuntu) * 3 (node 8, 10, 12). Looking through the renovate PRs, about 1/3rd of them randomly failed.
    Also seems that GH runs up to ± 6 builds at the same time, so in total it takes 15m-20m to check everything
  • Deps are completely version locked in package.json, which I think makes sense for the MDX library, but it creates noise for dev-dependencies, examples, and the site: I find the current list of open PRs and list of commits pretty unreadable
  • Our tests are based on fixtures, which is fine, but if something meaningless changes it causes a failure

While tooling is great (hey, I spend all my time building tools), and maybe I’m just horribly out of touch with what modern development is supposed to look like, I think it would help maintainers and contributors if we could make the repo simpler and builds faster and less fragile! A couple of ideas:

  • Don’t build on macOS: it’s mostly similar to ubuntu and I believe most maintainers of MDX have a mac
  • Don’t build on Node versions except for the outliers (so at the moment, do test on 8 and 12, but not on 10)
  • Marie Kondō dev-dependencies / tools
  • Move remark-* stuff to the @remarkjs org
  • Figure out if we can configure renovate better
  • Rework tests! (this is a big thing that should be discussed somewhere else)
@wooorm wooorm added 💬 type/discussion This is a request for comments 👩‍⚕ area/health This affects community 🏡 area/internal This affects the hidden internals labels Apr 14, 2020
@ChristianMurphy
Copy link
Member

but it takes 2:30 to install dependencies

npm 7 may help, v7 adds support for workspaces which was previously one of the main draws for yarn.
Testing locally a clean yarn install:

$ rm -rf **/node_modules
$ node --version
v15.1.0
$ yarn --version
1.22.10
$ yarn

takes around 59-68 seconds.
while a clean npm install

$ rm -rf **/node_modules
node --version
v15.1.0
$ npm --version
7.0.8
$ npm ci

takes between 42 to 48 seconds.

I expect the results would be even more pronounced on Github Actions.

between 2 and 4 minutes to store a cache version of them

Cache time may be mostly unavoidable

but, tying into:

Deps are completely version locked

Allowing version ranges could allow the version resolver some more flexibility in de-duplicating.

Don’t build on macOS: it’s mostly similar to ubuntu and I believe most maintainers of MDX have a mac

I'd push back on this a bit, while uncommon, there can be some MacOS specific issues.
I've been leaning more towards T shaped testing lately

MacOS Linux Windows
Latest ✔️ ✔️ ✔️
LTS ✔️
LTS 2 ✔️

Which tests across all OSes, but only tests previous Node versions in a specific OS.

@ChristianMurphy
Copy link
Member

Marie Kondō dev-dependencies / tools

Do you have some specific tools in mind, which don't spark joy anymore?

Move remark-* stuff to the @remarkjs org

👍

@ChristianMurphy
Copy link
Member

Figure out if we can configure renovate better

Renovate has some pretty powerful configuration options.
Do you have some particular ideas in mind?
https://docs.renovatebot.com/noise-reduction articulates some common techniques for noise reduction.

Another which lines up with possibly loosening:

Deps are completely version locked

would be ":preserveSemverRanges"

wooorm added a commit that referenced this issue Dec 18, 2020
This updates the dependencies and dev-dependencies in `packages/`.
Unfortunately, either updating to webpack 5 or updating to react 17 crash the
webpack loader with a react error, with an [invalid hook call
warning](https://reactjs.org/warnings/invalid-hook-call-warning.html) for
`useMDXComponents`:

<https://github.com/mdx-js/mdx/blob/dafdf6d70affa5dba0b3b7070f7a310b70bbf775/packages/react/src/context.js#L15>

Which might have to do with the magic of shortcodes (#1385), or something else,
I have no clue.

Furthermore, this loosens package dependencies instead of locking them,
which relates to GH-865, GH-1015, and GH-1267.
It was a long and divided discussion before and the reason for changing now is:
While the package currently doesn’t break easily (it was mentioned that unlocking
packages might cause that), we are currently *locked* on security vulnerabilities.
We’re not getting any patches, and MDX isn’t released that frequently or
maintained that actively, so MDX users are stuck.
If folks want to lock: npm and yarn have package locks.

Closes GH-1267.
Closes GH-1375.
wooorm added a commit that referenced this issue Dec 18, 2020
This updates the dependencies and dev-dependencies in `packages/`.
Unfortunately, either updating to webpack 5 or updating to react 17 crash the
webpack loader with a react error, with an [invalid hook call
warning](https://reactjs.org/warnings/invalid-hook-call-warning.html) for
`useMDXComponents`:

<https://github.com/mdx-js/mdx/blob/dafdf6d70affa5dba0b3b7070f7a310b70bbf775/packages/react/src/context.js#L15>

Which might have to do with the magic of shortcodes (#1385), or something else,
I have no clue.

Furthermore, this loosens package dependencies instead of locking them,
which relates to GH-865, GH-1015, and GH-1267.
It was a long and divided discussion before and the reason for changing now is:
While the package currently doesn’t break easily (it was mentioned that unlocking
packages might cause that), we are currently *locked* on security vulnerabilities.
We’re not getting any patches, and MDX isn’t released that frequently or
maintained that actively, so MDX users are stuck.
If folks want to lock: npm and yarn have package locks.

Closes GH-1267.
Closes GH-1375.
wooorm added a commit that referenced this issue Dec 20, 2020
This updates the dependencies and dev-dependencies in `packages/`.
Unfortunately, either updating to webpack 5 or updating to react 17 crash the
webpack loader with a react error, with an [invalid hook call
warning](https://reactjs.org/warnings/invalid-hook-call-warning.html) for
`useMDXComponents`:

<https://github.com/mdx-js/mdx/blob/dafdf6d70affa5dba0b3b7070f7a310b70bbf775/packages/react/src/context.js#L15>

Which might have to do with the magic of shortcodes (#1385), or something else,
I have no clue.

Furthermore, this loosens package dependencies instead of locking them,
which relates to GH-865, GH-1015, and GH-1267.
It was a long and divided discussion before and the reason for changing now is:
While the package currently doesn’t break easily (it was mentioned that unlocking
packages might cause that), we are currently *locked* on security vulnerabilities.
We’re not getting any patches, and MDX isn’t released that frequently or
maintained that actively, so MDX users are stuck.
If folks want to lock: npm and yarn have package locks.

Closes GH-1267.
Closes GH-1375.
Closes GH-1392.
wooorm added a commit that referenced this issue Dec 28, 2020
Create React App is the most looked at resource by users here on GitHub.
But it’s suggesting an old, unmaintained, and buggy way to use MDX.

This instead updates the guide to use our maintained projects, without
having to eject from CRA.

As CRA itself is an ever-changing “init” tool, which can support MDX by
following a couple steps, I don’t think it’s wise to have an example in
the project: we want folks to do `npx create-react-app ...`, not clone
our custom example.

Not having CRA checked in also makes for a faster `yarn install`.

Perhaps developing our own [CRA
template](https://create-react-app.dev/docs/custom-templates) might be
nice for the future, but for now I’ve kept it at an up to date and
working guide.

Related to GH-1015.
Related to GH-1388.

Closes GH-365.
Closes GH-589.
wooorm added a commit that referenced this issue Dec 28, 2020
Create React App is the most looked at resource by users here on GitHub.
But it’s suggesting an old, unmaintained, and buggy way to use MDX.

This instead updates the guide to use our maintained projects, without
having to eject from CRA.

As CRA itself is an ever-changing “init” tool, which can support MDX by
following a couple steps, I don’t think it’s wise to have an example in
the project: we want folks to do `npx create-react-app ...`, not clone
our custom example.

Not having CRA checked in also makes for a faster `yarn install`.

Perhaps developing our own [CRA
template](https://create-react-app.dev/docs/custom-templates) might be
nice for the future, but for now I’ve kept it at an up to date and
working guide.

Related to GH-1015.
Related to GH-1388.

Closes GH-365.
Closes GH-589.
wooorm added a commit that referenced this issue Dec 29, 2020
Create React App is the most looked at resource by users here on GitHub.
But it’s suggesting an old, unmaintained, and buggy way to use MDX.

This instead updates the guide to use our maintained projects, without
having to eject from CRA.

As CRA itself is an ever-changing “init” tool, which can support MDX by
following a couple steps, I don’t think it’s wise to have an example in
the project: we want folks to do `npx create-react-app ...`, not clone
our custom example.

Not having CRA checked in also makes for a faster `yarn install`.

Perhaps developing our own [CRA
template](https://create-react-app.dev/docs/custom-templates) might be
nice for the future, but for now I’ve kept it at an up to date and
working guide.

Related to GH-1015.
Related to GH-1388.

Closes GH-365.
Closes GH-589.
Closes GH-1422.

Reviewed-by: Christian Murphy <christian.murphy.42@gmail.com>
wooorm added a commit that referenced this issue Sep 14, 2021
* yarn is having problems hoisting dependencies
* workspaces are built into npm now
* GHA + npm have some fancy caching mechanisms

Reviewed-by: Christian Murphy <christian.murphy.42@gmail.com>

Related-to: GH-1015.
wooorm added a commit that referenced this issue Sep 15, 2021
This tests on all OSs (adding macOs) on latest Node & removes the Windows test on older nodes (latest Node is still tested).

Related-to: #1015.
@wooorm
Copy link
Member Author

wooorm commented Oct 19, 2021

solved, actions are much faster, the repo is installable, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👩‍⚕ area/health This affects community 🏡 area/internal This affects the hidden internals 💬 type/discussion This is a request for comments
Development

No branches or pull requests

2 participants