-
Notifications
You must be signed in to change notification settings - Fork 292
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
chore(rollup): upgrade rollup deps, migrate to mjs config file, enforce linting on mjs files #5830
Conversation
…ce linting on mjs files
🦋 Changeset detectedLatest commit: 1336147 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Just 2 questions, but if they are intended, I am GTG
packages/react-native/src/Authenticator/__tests__/withAuthenticator.spec.tsx
Show resolved
Hide resolved
extends: ['@aws-amplify/amplify-ui/react'], | ||
ignorePatterns: ['.eslintrc.js', 'dist', 'rollup.config.ts'], | ||
extends: '@aws-amplify/amplify-ui/react', | ||
ignorePatterns: ['.eslintrc.js', 'coverage', 'dist', 'node_modules'], |
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.
Linting has been expanded to cover top level .ts and .mjs files. Added coverage
and node_modules
to the ignorePatterns
to mitigate extraneous linting
// point to local tsconfig | ||
parserOptions: { project: ['tsconfig.json'], tsconfigRootDir: __dirname }, | ||
overrides: [ | ||
{ | ||
extends: ['@aws-amplify/amplify-ui/jest'], | ||
files: ['**/__mocks__/**', '**/__tests__/**'], | ||
}, | ||
{ files: '*.mjs' }, |
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.
Captures top level .mjs files (basically just rollup.config.mjs
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.
Does this do anything if it's not paired with a rule?
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.
It runs the default set of rules
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.
Do the default rules not get run by default for those files?
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.
They were not but this could potentially be removed as some changes were made after it was added, either way preference is not to make any changes to this PR unless they impact the bundle output
@@ -2,5 +2,5 @@ const config = require('../../.lintstagedrc.js'); | |||
|
|||
module.exports = { | |||
...config, | |||
'*.{ts,tsx,js}': 'eslint', | |||
'*.{ts,tsx,js,mjs}': 'eslint', |
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.
Lint .mjs files on commit
@@ -34,7 +34,7 @@ | |||
"check:esm": "node --input-type=module --eval 'import \"@aws-amplify/ui-react-ai\"'", | |||
"clean": "rimraf dist node_modules", | |||
"dev": "yarn build:rollup --watch", | |||
"lint": "yarn typecheck && eslint src", | |||
"lint": "yarn typecheck && eslint .", |
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.
src
-> .
to allow linting of top level config files
@@ -1,4 +1,3 @@ | |||
import { ComponentStyles } from './utils'; | |||
|
|||
export type ButtonGroupTheme<Required extends boolean = false> = | |||
ComponentStyles; | |||
export interface ButtonGroupTheme extends ComponentStyles {} |
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.
Linting fix due to new rules (ref)
@@ -1,4 +1,3 @@ | |||
import { ComponentStyles } from './utils'; | |||
|
|||
export type CheckboxFieldTheme<Required extends boolean = false> = | |||
ComponentStyles; | |||
export interface CheckboxFieldTheme extends ComponentStyles {} |
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.
Linting fix due to new rules (ref)
@@ -1,4 +1,3 @@ | |||
import { ComponentStyles } from './utils'; | |||
|
|||
export type SelectFieldTheme<Required extends boolean = false> = | |||
ComponentStyles; | |||
export interface SelectFieldTheme extends ComponentStyles {} |
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.
Linting fix due to new rules (ref)
import { DefaultTheme, WebTheme } from '../types'; | ||
import { WebTheme } from '../types'; |
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.
Linting fix due to new rules (ref)
@@ -1,4 +1,5 @@ | |||
{ | |||
"extends": "./tsconfig.json", | |||
"include": ["src/**/*.ts"], |
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.
Ensures that only .ts files under src are included in build output
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.
Most thorough comment walk-through in a PR I've seen in a while. Kudos!
Co-authored-by: Danny Banks <djb@amazon.com>
Description of changes
Upgrade
rollup
from^2.70.0
to^4.22.4
(ref). The approach taken by this PR configuresrollup
to emit a bundled output that aligns as closely as possible with the current output. While some of the flags may no longer be necessary, additional modifications to bundle output should be handled in a major version bump.See this commit for a diff of the dist output between existing bundle output and the output of the changes from this PR.
Summary of changes:
@aws-amplify/react-core/elements
ESM subpathsIssue #, if available
NA
Description of how you validated changes
Manual tests, visual diffing of bundle output diff, CI workflows taken against this PR
Checklist
yarn test
passes and tests are updated/addeddocs
,e2e
,examples
, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.