test(build system): add React tests for loadtest #3703
Conversation
0618
changed the title
| @@ -0,0 +1,35 @@ | |||
| { | |||
There was a problem hiding this comment.
Was this file autogenerated?
There was a problem hiding this comment.
It was modified from the auto-generated nextjs 13
| } from '@aws-amplify/ui-react'; | ||
| import { StorageManager } from '@aws-amplify/ui-react-storage'; | ||
| import '@aws-amplify/ui-react/styles.css'; | ||
| import awsconfig from '@/data/aws-exports'; |
There was a problem hiding this comment.
Can we use relative path instead of @/?
There was a problem hiding this comment.
@/ was from the auto-generated config. Since we try to mimic the users behavior, it might be better not change it
There was a problem hiding this comment.
Understood, not blocking then.
There was a problem hiding this comment.
I'm good with leaving tsconfig as-is then, but let's use relative path in the import statement, since that's what we use in docs.
There was a problem hiding this comment.
That's a good catch. The newer version of Next.js auto generated the relative path as default, so I kept it here
| @@ -0,0 +1,13 @@ | |||
| // @ts-nocheck | |||
There was a problem hiding this comment.
Why is @ts-nocheck necessary?
There was a problem hiding this comment.
otherwise there would be a lot of red marks in VS Code.
That might be a good point. Would that prevent some necessary build failures if there's any TS issue?
There was a problem hiding this comment.
since we're interested in catching TS errors, let's remove it.
| <AccountSettings.ChangePassword onSuccess={() => {}} /> | ||
| <AccountSettings.DeleteUser onSuccess={() => {}} /> | ||
| <StorageManager | ||
| acceptedFileTypes={['image/*']} |
There was a problem hiding this comment.
Nit, but spacing seems off. Can you run prettier?
| push: | ||
| branches: [main] # Run when merge to main |
There was a problem hiding this comment.
Should we run this on push? I don't see much value in this, because build-system-tests will test against @latest, not the commit that was just merged.
There was a problem hiding this comment.
Oh, that might be my mis-understanding. I thought push on main is publish to @latest 😂
There was a problem hiding this comment.
push on main is by default publish to @next unless the commit is from "Version Packages" PR. In either way, we would have to wait until publish-next finishes to run build test against it.
build-system-tests/templates/components/react/next/template-tsconfig-11.json
Show resolved
Hide resolved
| "next": "^11.1.3", | ||
| "react": "17", | ||
| "react-dom": "17", | ||
| "typescript": "5.0.3" |
There was a problem hiding this comment.
Is typescript meant to be pinned?
There was a problem hiding this comment.
Good catch. I'll test and address it in the real PR
| {({ signOut, user = { username: '' } }) => ( | ||
| <main> | ||
| <h1>Hello {user.username}</h1> |
There was a problem hiding this comment.
What about
| {({ signOut, user = { username: '' } }) => ( | |
| <main> | |
| <h1>Hello {user.username}</h1> | |
| {({ signOut, user }) => ( | |
| <main> | |
| <h1>Hello {user.username}</h1> |
to mimic our documentation code?
There was a problem hiding this comment.
or user?.username if above isn't TS strict compliant.
There was a problem hiding this comment.
Our docs would have a type error. We have to either set a default value or add a question mark like user?.username 😂
There was a problem hiding this comment.
Let's add a question mark for now. Let's add a ticket for updating our docs for TS strict customers.
Description of changes
TODO:
Issue #, if available
Description of how you validated changes
Tested on fork's build:
PoC PR on fork: 0618#20
PoC PR in this repo: #3751
Checklist
yarn testpassessideEffectsfield updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.