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

Upgrade Parcel and other dev dependencies #2701

Merged
merged 29 commits into from
Dec 22, 2021
Merged

Upgrade Parcel and other dev dependencies #2701

merged 29 commits into from
Dec 22, 2021

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Dec 20, 2021

This updates a number of our dev dependencies, with the goal of improving build performance and fixing bugs.

  • Parcel, to the latest nightly. This improves build performance significantly, and fixes an issue with the docs examples that we disabled optimizations for last summer by using the new experimental bundler. It also uses the new Rust-based JS compiler rather than Babel.
  • MDX to v2. This is significantly faster than the old version of MDX since it doesn't rely on Babel anymore.
  • React to 17 by default. We still build a 16 storybook in CI, but the default version is now 17. This is because MDX v2 requires React 17, and also because, it's time.
  • TypeScript to 4.x. Also due to MDX.
  • Node to 14.x in CI due to all of the above.
  • Replaces Prettier with dprint-node, which is much faster. This was used for auto formatting examples in the docs.

See explanations for additional things inline.

Overall, build performance improved significantly.

  • In dev, rebuilds went from 10+ seconds to ~4.
  • In CI, docs builds went from ~244s to ~28s. 🥳

@adobe-bot
Copy link

Build successful! 🎉

…grade

# Conflicts:
#	packages/@react-aria/grid/src/useGrid.ts
#	yarn.lock
@@ -1,19 +1,25 @@
{
"extends": "@parcel/config-default",
"resolvers": ["parcel-resolver-docs", "..."],
"resolvers": ["@parcel/resolver-glob", "parcel-resolver-docs", "..."],
"bundler": "@parcel/bundler-experimental",
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes the issue with some examples not rendering which we had patched Parcel before, disabling optimization. The new bundler is currently in progress and experimental, but it seems to build our docs correctly.

"transformers": {
"apiCheck:*.{js,ts,tsx,json}": ["parcel-transformer-docs"],
"docs:*.{js,ts,tsx,json}": ["parcel-transformer-docs", "@parcel/transformer-inline"],
"docs-json:*.{js,ts,tsx,json}": ["parcel-transformer-docs"],
"*.{md,mdx}": ["parcel-transformer-mdx-docs"],
"*.svg": ["@parcel/transformer-svg-react"],
"*.css": ["...", "parcel-transformer-css-env"]
"*.css": ["...", "parcel-transformer-css-env"],
"*.{js,mjs,jsm,jsx,es6,cjs,ts,tsx}": [
Copy link
Member Author

Choose a reason for hiding this comment

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

This disables Babel from running in Parcel, even though we have a .babelrc. Parcel includes a much faster transpiler now. Eventual goal is to also use it for Jest and Storybook and remove Babel completely.

],
"@babel/plugin-proposal-export-default-from",
"@babel/plugin-proposal-export-namespace-from",
"@babel/plugin-syntax-class-properties",
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 are included in @babel/preset-env. No need to duplicate them here.

@@ -1,11 +1,11 @@
<!-- Copyright 2020 Adobe. All rights reserved.
{/* Copyright 2020 Adobe. All rights reserved.
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment format changed from HTML style to JSX style in MDX v2.

@@ -33,9 +34,9 @@ description: A library of React Hooks that provides cross-platform state managem
A library of React Hooks that provides cross-platform state management for your design system.

<p className={clsx(typographyStyles['spectrum-Body2'], styles.homeLinks)}>
<a href="getting-started.html">Get started</a>
<a href="getting-started.html" className={clsx(linkStyle['spectrum-Link'], styles.link)}>Get started</a>
Copy link
Member Author

Choose a reason for hiding this comment

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

MDX v2 no longer seems to apply the components mapping to explicit elements like this, only to markdown elements (e.g. [some link](url)).

Copy link

Choose a reason for hiding this comment

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

@devongovett do let mdx folks know if this is weird, or kinda makes sense. I’m not 100% on this change.

Copy link
Member Author

@devongovett devongovett Dec 22, 2021

Choose a reason for hiding this comment

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

@wooorm 👋. I think it kinda makes sense? It seems MDX v2 is moving more toward JSX and away from Markdown/HTML (e.g. comment syntax as well). In JSX the way to solve this is using components, rather than overriding how HTML elements are rendered. It was a convenient feature, but certainly not required.

@@ -0,0 +1,77 @@
diff --git a/node_modules/@parcel/transformer-postcss/lib/PostCSSTransformer.js b/node_modules/@parcel/transformer-postcss/lib/PostCSSTransformer.js
Copy link
Member Author

Choose a reason for hiding this comment

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

This patch is necessary because we currently require a very old version of PostCSS which is no longer supported by Parcel.

@adobe-bot
Copy link

Build successful! 🎉

"react-axe": "^3.0.2",
"react-dom": "^16.8.0 || ^17.0.0-rc.1",
"react-dom": "^17.0.2",
"react-overlays": "0.8.3",
"react-test-renderer": "^16.9.0",
Copy link
Member

Choose a reason for hiding this comment

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

should we update this as well?
i'm not sure why it wasn't included in the install-17 script

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we even use it? I don't see any references. And the tests seem to pass?

Comment on lines +120 to +121
pageStyles.spectrum,
Object.values(theme.global),
Copy link
Member

Choose a reason for hiding this comment

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

what happened here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Some of the classes we apply in provider didn't seem to be applied here, which resulted in some broken styles. Honestly not sure how it was working before. 😂

Copy link
Member

Choose a reason for hiding this comment

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

Taking a look at the diffs, <kbd> didn't get the keyboard class:
Old:
Old
New:
New

},
"dependencies": {
"@parcel/plugin": "nightly"
"@parcel/plugin": "2.0.0-nightly.951"
Copy link
Member

Choose a reason for hiding this comment

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

what's with the mismatch in nightlies?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤷 engines has to match the @parcel/core version. Not sure why @parcel/plugin is different. Lerna does weird things.

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

snowystinger
snowystinger previously approved these changes Dec 21, 2021
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Thank you!

reidbarber
reidbarber previously approved these changes Dec 21, 2021
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -98,6 +98,7 @@ export function useColorField(
mergeProps(props, {
id: inputId,
value: inputValue,
defaultValue: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Member

Choose a reason for hiding this comment

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

the TS upgrade found this

@@ -97,6 +97,6 @@ import LockClosed from '@spectrum-icons/workflow/LockClosed';
A [searchable list of workflow icons](https://spectrum.adobe.com/page/icons/) is available on the Spectrum website.
The name of the icon without any whitespace matches the import in React Spectrum.

<!-- ```tsx snippet
{/* ```tsx snippet
Copy link
Member

Choose a reason for hiding this comment

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

Should we delete this?

Copy link
Member

Choose a reason for hiding this comment

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

it's not currently hurting anything, so I'm ok with it staying for the moment

@@ -296,7 +296,7 @@ function addPerson(name) {
</ListBox>
```

<!--
{/*
Copy link
Member

Choose a reason for hiding this comment

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

Why are these commeted out instead of deleted?

Copy link
Member

Choose a reason for hiding this comment

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

it was a comment before, this is just the way that mdx comments need to be now, see #2701 (comment)

@@ -19,7 +19,10 @@ import ShowMenu from '@spectrum-icons/workflow/ShowMenu';
import {ThemeSwitcher} from './ThemeSwitcher';
import {watchModals} from '@react-aria/aria-modal-polyfill';

window.addEventListener('load', () => listen());
if (process.env.NODE_ENV === 'production') {
Copy link
Member

Choose a reason for hiding this comment

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

This does seem like a feature for production, but wondering if the upgrade caused this or you thought this would be useful while working on parcel upgrade?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, originally I had enabled Parcel's lazy mode which only builds pages that you actually load in the browser, and the preloading was causing us to build more than necessary. I took out lazy, but this still seems like a production only feature anyway so I left it.

@devongovett devongovett dismissed stale reviews from reidbarber and snowystinger via 908b98c December 22, 2021 18:33
@adobe-bot
Copy link

Build successful! 🎉

@ktabors
Copy link
Member

ktabors commented Dec 22, 2021

LGTM because: locally I ran storybook, all the default stories worked for all components. Lint and tests "passed" too. Tests had a similar know local fail as before. Build completed, but build:start still doesn't work for me, the error is different so I need to figure that out still.

snowystinger
snowystinger previously approved these changes Dec 22, 2021
@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

one of these times :)

@devongovett devongovett merged commit 7a5e585 into main Dec 22, 2021
@devongovett devongovett deleted the parcel-upgrade branch December 22, 2021 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants