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

chore(805): nx and rspack on existing project structure #847

Merged
merged 19 commits into from
Apr 28, 2023

Conversation

JohnTParsons
Copy link
Collaborator

@JohnTParsons JohnTParsons commented Apr 21, 2023

close #805

What

  1. Background - speed up
  2. What did you do - added nx and rspack on existing project structure
  3. What does the reviewers should expect - faster lint and unit test

Please see the comments on #805 for CI speed differences (9x faster for lint but not test) and local speed differences (significantly faster for both lint and test, which should make for a better developer experience).

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

@pp-serviceaccount
Copy link
Collaborator

@JohnTParsons JohnTParsons force-pushed the chore/805-nx-rspack branch 3 times, most recently from 10b483f to 54e37f5 Compare April 26, 2023 17:53
@JohnTParsons JohnTParsons marked this pull request as ready for review April 27, 2023 16:02
@JohnTParsons JohnTParsons added the ready for review Please assist in getting this reviewed label Apr 28, 2023
mutebg
mutebg previously approved these changes Apr 28, 2023
"declaration": true,
"declarationMap": true,
"moduleResolution": "node",
Copy link
Contributor

Choose a reason for hiding this comment

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

why all these new rules 🤔 ?

Copy link
Collaborator Author

@JohnTParsons JohnTParsons Apr 28, 2023

Choose a reason for hiding this comment

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

They are what the command to migrate to nx (npx add-nx-to-monorepo) added

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be wise to do some testing on a pre-release to check that these changes haven't had any impact on the built package.

package.json Show resolved Hide resolved
@JohnTParsons JohnTParsons merged commit 5e29c58 into main Apr 28, 2023
@JohnTParsons JohnTParsons deleted the chore/805-nx-rspack branch April 28, 2023 14:24
Comment on lines +12 to +19
"src/**/*.test.ts",
"src/**/*.spec.ts",
"src/**/*.test.tsx",
"src/**/*.spec.tsx",
"src/**/*.test.js",
"src/**/*.spec.js",
"src/**/*.test.jsx",
"src/**/*.spec.jsx",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"src/**/*.test.ts",
"src/**/*.spec.ts",
"src/**/*.test.tsx",
"src/**/*.spec.tsx",
"src/**/*.test.js",
"src/**/*.spec.js",
"src/**/*.test.jsx",
"src/**/*.spec.jsx",
"src/**/*.{test,spec}.{ts,tsx,js,jsx}"

Comment on lines +9 to +21
"overrides": [
{
"files": ["*.ts", "*.tsx", "*.js", "*.jsx"],
"rules": {}
},
{
"files": ["*.ts", "*.tsx"],
"rules": {}
},
{
"files": ["*.js", "*.jsx"],
"rules": {}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

are these overrides obsolete ass they don't have any rules?

"development": {
"outputPath": "site"
},
"production": {}
Copy link
Contributor

Choose a reason for hiding this comment

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

is an output path required here?

@@ -0,0 +1,12 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we have to introduce babel? do wonder if this will slow things down...

Comment on lines +18 to +25
{
"files": ["*.ts", "*.tsx"],
"rules": {}
},
{
"files": ["*.js", "*.jsx"],
"rules": {}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

again, not sure these are doing anything.

"lint:markdown": "nx exec -- markdownlint -c ./.markdownlint.json *.md **/*.md",
"lint:next": "nx exec -- next lint site",
"lint:packages": "npx nx report",
"lint:legacy": "concurrently --names \"code,markdown,next\" \"scripts/assert-files-and-folders.js && yarn lint:code:base .\" \"yarn markdownlint -c ./.markdownlint.json *.md **/*.md\" \"yarn next lint site\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

suspect these legacy ones will be removed?

Comment on lines +11 to +21
"files": ["*.ts", "*.tsx", "*.js", "*.jsx"],
"rules": {}
},
{
"files": ["*.ts", "*.tsx"],
"rules": {}
},
{
"files": ["*.js", "*.jsx"],
"rules": {}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

are these required?

"test:legacy": "yarn lint:legacy && yarn test:unit:dev:legacy",
"test:unit:node": "node --max-old-space-size=8192 --expose-gc ./node_modules/.bin/jest --logHeapUsage --coverage --coverageReporters=lcov --coverageReporters=text",
"test:unit:watch": "yarn test:unit:node --maxWorkers=50% --watch",
"test:unit:run": "NODE_OPTIONS=\"--max-old-space-size=8192\" nx exec -- jest --logHeapUsage --coverage --coverageReporters=lcov --coverageReporters=text",
Copy link
Contributor

Choose a reason for hiding this comment

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

given you've bumped the CI instance size should this memory size go up also?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nx and rspack on existing project structure
5 participants