-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Runtime auto #8643
Runtime auto #8643
Conversation
### WHY are these changes introduced? Removes a deprecated argument on the Collapsible component with the major version bump. ### WHAT is this pull request doing? Removes the prop as a breaking change. ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) N/A tophatting instructions as Polaris does not use this prop internally. ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
…7990) ### WHY are these changes introduced? Breadcrumbs has only been using a single breadcrumb for a while now. Instead of allowing consumers to pass an array without knowing why only a single link is being used, we should remove the ability to pass an array and make the component easier to understand implicitly. ### WHAT is this pull request doing? UI will not change. The Breadcrumbs component was already only using the last breadcrumb link in the array. This PR changes the props for breadcrumbs and also the implementation on the Page component. ### Migration path Since this PR removes the `breadcrumbs[]` prop in favor of `breadcrumb` all uses of Page, and Breadcrumbs will need to be migrated. It will look something like this: ```ts //before const breadcrumbs = [ { content: 'Products', url: 'http://test.com', }, ]; <Page {...mockProps} breadcrumbs={breadcrumbs} /> //after const breadcrumb = { content: 'Products', url: 'http://test.com', }; <Page {...mockProps} breadcrumb={breadcrumb} /> ``` ### How to 🎩 Check the Breadcrumbs and Page components in storybook ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
### WHY are these changes introduced? Now that the admin has been migrated to use Text, we can mark `Text` as stable in v11. ### WHAT is this pull request doing? Removes beta status and message banner from `Text` component page. <details> <summary>Update Text component page</summary> <img src="https://user-images.githubusercontent.com/26749317/215123915-0ed9fef9-d4f7-413a-a31e-73d4e80fec28.png" alt="Update Text component page"> </details> <!-- ℹ️ Delete the following for small / trivial changes --> ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
### WHY are these changes introduced? Resolves #6511. Now that `Shopify/web` has been migrated to use the `Text` component, we can safely remove the 6 deprecated typography components. Note: The 3 chromatic changes are related to other changes in the v11 branch and not this PR. ### WHAT is this pull request doing? Removes `DisplayText`, `Heading`, `Subheading`, `Caption`, `TextStyle`, and `VisuallyHidden`. <!-- ℹ️ Delete the following for small / trivial changes --> ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) <!-- Give as much information as needed to experiment with the component in the playground. --> <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import React from 'react'; import {Page} from '../src'; export function Playground() { return ( <Page title="Playground"> {/* Add the code you want to test in here */} </Page> ); } ``` </details> ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
### WHY are these changes introduced? Rollup and Babel are blocking our migration to minimum NodeJS version 16. This PR updated all Rollup and Babel dependencies. These NPM package upgrades are not compatible with NodeJS 14.17 (main branch version). This change unblocks a larger change to our NodeJS versions. ### WHAT is this pull request doing? - [x] Moves duplicate dependencies to root - [x] Upgrades all rollup and babel dependencies - [x] Fixes any required breaking changes or warnings - [x] Compared diff of previous packages and new ones and it doesn't have a regression ### How to 🎩 1. Run a build on `main` save the built files on desktop 1. Run a build on this `bump-build-deps` save the built files on desktop 1. Run a diff over the built files 1. Make sure no regressions 1. CI, tests should pass and website should render all pages. **@alex-page's output from Diff:** [diff.zip](https://github.com/Shopify/polaris/files/10531068/diff.zip) ``` diff -bur polaris-tokens/dist-main polaris-tokens/dist-v11 > icons.diff diff -bur polaris-tokens/dist-main polaris-tokens/dist-v11 > tokens.diff diff -bur polaris-migrator/dist-main polaris-migrator/dist-v11 > migrator.diff diff -bur polaris-react/build-main polaris-react/build-v11 > react.diff ``` ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [x] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
### WHY are these changes introduced? NodeJS 14 is EOL and we are removing support in v11. We currently set `engines` on the root `package.json`. The problem with this is that consumers of the library do not get this information and they can use the libraries without any warning of what version of NodeJS it supports. ### WHAT is this pull request doing? - [x] Increasing the supported versions of NodeJS - [x] Add the engines field for NodeJS to package.json files I would like to get NodeJS to version 16.19 before we launch version 11. However there is an issue with babel/rollup/browserslist not finding the latest version of NodeJS: ``` (plugin babel) BrowserslistError: Unknown version 16.19 of Node.js ``` ### How to 🎩 - CI completes successfully
…8211) Co-Authored-By: Ben Scott <[email protected]>
### WHY are these changes introduced? Re-adds the deprecated typography docs and images on the style guide so that users can still reference them. The `get-props` script had to be updated to resolve build errors. It was failing since the component files no longer existed in `polaris-react/src/components`. ### WHAT is this pull request doing? - Re-add dep components images - Re-add dep component pages - ~Adds new `deprecatedComponents` array to the `get-props` file and if the parsed file matches any of the elements, return empty object~ - If component status is deprecated, props table is not rendered on component page <!-- ℹ️ Delete the following for small / trivial changes --> ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) <!-- Give as much information as needed to experiment with the component in the playground. --> <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import React from 'react'; import {Page} from '../src'; export function Playground() { return ( <Page title="Playground"> {/* Add the code you want to test in here */} </Page> ); } ``` </details> ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
### WHY are these changes introduced? Polaris currently supports multiple TypeScript versions by building our types twice. This breaking change in v11 removes this functionality and we will only support the current version in the library. This used to be required to build the props table on legacy.polaris.shopify.com. This is no longer a requirement with @martenbjork brilliant work. ### WHAT is this pull request doing? Removes legacy type support for the library. --------- Co-authored-by: Ben Scott <[email protected]> Co-authored-by: Aaron Casanova <[email protected]>
### WHY are these changes introduced? Resolves #8102. Replaces deprecated `Stack` component with `AlphaStack`. ### WHAT is this pull request doing? - Replaces deprecated Stack with AlphaStack in `polaris-react` - Updates examples and components using AlphaStack to use Stack - Removes deprecated Stack documentation and examples - Updates AlphaStack documentation and examples ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
This commit is a non-breaking change. There are slight differences with how class properties are initialised - it removes setting them to undefined before reassigning them, which isn't needed.
Now we transpile based upon our browserslist, we don't force the usage of these transpilations. In practice, given our current browserslist config this means that the following transforms are now no longer ran (for instance our compiled output now contains `foo?.bar` instead of that being transformed): - proposal-numeric-separator - proposal-nullish-coalescing-operator - proposal-optional-chaining
### WHY are these changes introduced? This PR replaces using `@shopify/babel-preset` with using the underlying presets directly. As part of this we simplify the plugins we use and lean more heavily on `@babel/preset-env`. As a result of this we no longer transpile numeric separators (`1_000`), nullish coalescing operators (`foo ?? 'bar'`) and optional chaining `foo?.bar` because our browserslist targets say that those features are all supported in the browsers that we target. This means that our build output now contains optional chaining et al. A breaking change side-effect of this is that code that uses these features will no longer successfully parse in webpack4, as webpack4 uses an old version of acorn does not understand these syntaxes. Webpack5 uses a newer version of acorn that can read this just fine. ### WHAT is this pull request doing? Replace `@shopify/babel-config` with using `@babel/preset-env`, `@babel/preset-typescript` and `@babel/preset-react` directly. This PR is split into two commits. The first replaces `@shopify/babel-preset` with inline config, with minimal changes. The second removes the forced compilation of the 5 plugins that were always forced to be enabled. ### How to 🎩 - Check out the main branch of polaris in your `~/projects/polaris` directory and run `cd package`, `yarn`, `yarn run turbo run build --filter='!polaris.shopify.com' --force` to produce a clean build - Check out this branch of polaris in your `~/src/github.com/Shopify/polaris` directory and run `cd package`, `yarn`, `yarn run turbo run build --filter='!polaris.shopify.com' --force` to produce a build - In the polaris in the src directory run `touch out.txt; for PACKAGENAME in $(ls -1 . | grep '^polaris-'); diff -ru ~/projects/polaris/$PACKAGENAME $PACKAGENAME -x '*.tsbuildinfo' -x '*.d.ts.map' -x'.turbo' -x '*.esnext' >> out.txt` to compare the build folder output. Note that the differences are as follows: - A handful of places where a class property is initialised to `void 0`. In all cases either the value is already implicitly undefined, or is assigned to a value shortly afterwards - Optional chaining is now left intact - Nullish coalesing is now left intact - numeric separators are now left intact --------- Co-authored-by: Alex Page <[email protected]>
@@ -3,13 +3,13 @@ | |||
"compilerOptions": { | |||
"composite": true, | |||
"emitDeclarationOnly": true, | |||
"importsNotUsedAsValues": "error", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were a lot of build errors related to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors relating to this would occur because of code that uses import {SomeType}
instead of import type {SomeType}
.
I can't see the errors but stuff like this, where React becomes referenced as some globally accessible type, rather than imported from react
might be related.
In the past I've saw that the react codemod doesn't touch types when you do things like import React
and then type Blah = React.FunctionComponent
.
It would potentially be worth trying out my fork of the codemod that handles that better: reactjs/react-codemod#305
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep had a quick poke at it, It's because the codemod doesn't handle our "you should use import type
if you're only importing types" restriction either. That's fine, that's really the remit of some linting you run after the codemod anyway. My fork doesn't help with that either, so ignore that suggestion.
importsNotUsedAsValues: error
basically enforces "if you only use types from an import then it must use import type
. Previously import React, {SomeType} from 'react'
wasn't a violation - you used React at runtime so that import declaration was considered ok. With the automatic runtime you don't need the default export anymore, so the codemod transforms that code into import {SomeType} from 'react'
. And then typescript shouts at you because you're only accessing types but you're using import
, not import type
.
You've got two approaches here:
The low-key just-get-it-out-approach: Manually go adjust those import {SomeType} from 'react'
lines that TS complains about to import type {SomeType} from 'react'
. There's like 11 violating files that seems pretty easy if you want to style it out in this PR.
The somewhat sweatier approach: Go enable the @typescript-eslint/consistent-type-imports
and @typescript-eslint/consistent-type-exports
lint rules on the main branch, then rebase the v11 branch, then rebase this branch. These are autofixable rules that will force usage of import type
when you import a type. See https://typescript-eslint.io/blog/consistent-type-imports-and-exports-why-and-how for more context. This is actually a little stricter than the "importsNotUsedAsValues": "error",
stuff described above - the linter will change the existing usage of import React, {SomeType} from 'react'
to be import React from 'react'; import type {SomeType} from 'react'
. If you do this, then get this branch rebased atop of that change, then running eslint --fix
will pull these files into a shape that will keep tsc happy without needing any manual touch ups
@@ -242,7 +242,7 @@ export default [ | |||
}, | |||
], | |||
|
|||
external: ['react'], | |||
external: ['react', 'react/jsx-runtime'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried moving this to the external plugin. I was a little bit confused around if these then need to be added to the package.json as it seems to be looking there. For now I left it as is to try and get this working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be ok for this to be external:
['react/jsx-runtime'],`
import React from 'react'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is going to be unhappy with this change
WHY are these changes introduced?
Fixes #0000
WHAT is this pull request doing?
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx
:🎩 checklist
README.md
with documentation changes