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(amplify-graphiql-explorer): upgrade react-scripts #10228

Merged
merged 3 commits into from
Apr 21, 2022

Conversation

johnpc
Copy link
Contributor

@johnpc johnpc commented Apr 15, 2022

This PR updates react-scripts in amplify-graphiql-explorer.

This required:

  1. running yarn policies set-version 1.18 to allow appropriate lerna dependency behavior
  2. upgrading react-scripts to ^5.0.0
  3. ejecting from react-scripts to add polyfills to the webpack config necessary for the jsonwebtoken dependency
  4. adding a script to fixup the semantic-ui dependency at build time to work properly with webpack5
  5. using "skipLibCheck": true in our tsconfig setup to prevent builds from failing due to the new dependencies
  6. upgrading windows e2e test harness to use node 14, since new dependencies don't support node 12.

@johnpc johnpc force-pushed the run-e2e/xss/upgrade-cra branch 3 times, most recently from 660586f to 071e13a Compare April 18, 2022 02:25
@@ -0,0 +1,74 @@
const fs = require("fs");
const path = require("path");
const nodeModulesPath = path.resolve(`${process.cwd()}/../..`, "node_modules");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Builds were failing because of an incompatibility with semantic-ui and webpack5. A patch package was created to bridge the issue (Semantic-Org/Semantic-UI#7073 (comment)).

This script is a lightly modified version of that patch package modified to work with our lerna setup.

@@ -0,0 +1,104 @@
'use strict';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost all the changes in this PR come from running CRA's eject script, which is necessary to add the polyfills needed in this package.

modules: ['node_modules', paths.appNodeModules].concat(
modules.additionalModulePaths || []
),
fallback: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These fallbacks I added to this generated file to support stream, util, crypto and buffer required by the jsonwebtoken dependency in this package.

@@ -22,6 +22,7 @@

/* Strict Type-Checking Options */
"strict": true /* Enable all strict type-checking options. */,
"skipLibCheck": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typescript was having some problems digesting the fact that @types/eslint and @types/eslint-scope are both in the dependency tree of this package.

# yarn lockfile v1


yarn-path ".yarn/releases/yarn-1.18.0.cjs"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this silenced an error I was seeing when adding dependencies to this package

error An unexpected error occurred: "expected workspace package to exist for \"@babel/template\"".

Resolved by running yarn policies set-version 1.18 which generated these files. See yarnpkg/yarn#7807 for more

@johnpc johnpc changed the title chore: upgrade cra chore(amplify-graphiql-explorer): upgrade react-scripts Apr 18, 2022
@johnpc johnpc force-pushed the run-e2e/xss/upgrade-cra branch 5 times, most recently from 6a37036 to faab692 Compare April 20, 2022 19:53
@johnpc johnpc force-pushed the run-e2e/xss/upgrade-cra branch from faab692 to 6bf5744 Compare April 20, 2022 20:09
@johnpc johnpc merged commit fe73fa4 into master Apr 21, 2022
Copy link
Contributor

@sachscode sachscode left a comment

Choose a reason for hiding this comment

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

LGTM - changes are all localized to graphiql-explorer
Since this is GraphiQL related, it would be good to test this manually after clearing all local state.
( create-app + use graphiql + push + pull with amplify-tagged-release/dev + graphiql ).
we must test the same with datastore.

@aws-eddy aws-eddy deleted the run-e2e/xss/upgrade-cra branch February 18, 2023 03:03
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.

4 participants