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(dependencies): upgrade dependencies #1147

Merged
merged 18 commits into from
Jul 2, 2024

Conversation

pladaria
Copy link
Member

@pladaria pladaria commented Jun 11, 2024

Upgraded:

  • prettier
  • eslint
  • acceptance-testing
  • react-testing-library & co
  • typescript
  • vanilla extract
  • playroom (as requested by @yceballost )

Copy link

github-actions bot commented Jun 11, 2024

Size stats

master this branch diff
Total JS 11.9 MB 11.9 MB +1.94 kB
JS without icons 1.97 MB 1.97 MB +1.94 kB
Lib overhead 54.2 kB 54.2 kB +34 B
Lib overhead (gzip) 14.5 kB 14.6 kB +17 B

Copy link

github-actions bot commented Jun 11, 2024

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

Copy link

github-actions bot commented Jun 11, 2024

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-qg8slgg5b-flows-projects-65bb050e.vercel.app

Built with commit e87fc41.
This pull request is being automatically deployed with vercel-action

@pladaria pladaria requested review from atabel and marcoskolodny June 11, 2024 15:29
@pladaria pladaria marked this pull request as ready for review June 11, 2024 15:29
@pladaria pladaria marked this pull request as draft June 11, 2024 16:19
@@ -10,8 +11,8 @@ module.exports = {
'!**/__*__/**', // ignore tests, acceptance, stories, etc
],
transform: {
'\\.css\\.ts$': '@vanilla-extract/jest-transform',
'^.+\\.(t|j)sx?$': ['@swc/jest', {...swcConfig, sourceMaps: 'inline'}],
'\\.css\\.ts$': ['@vanilla-extract/jest-transform', swcTransform],
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason, the swc transform is now required in the css.ts files.
It makes sense but I'm not sure why this was working before.

setupFilesAfterEnv: [require.resolve('./setup-test-env.tsx'), '@testing-library/jest-dom/extend-expect'],
setupFilesAfterEnv: ['<rootDir>/setup-test-env.tsx'],
Copy link
Member Author

Choose a reason for hiding this comment

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

the setup-test-env now imports the jest-dom module
https://github.com/testing-library/jest-dom?tab=readme-ov-file#usage

Comment on lines 161 to 163
await userEvent.click(screen.getByTestId('dialog-overlay'));
userEvent.click(screen.getByTestId('dialog-overlay'));

await waitForElementToBeRemoved(() => screen.queryByRole('dialog', {hidden: true}));
Copy link
Member Author

@pladaria pladaria Jun 12, 2024

Choose a reason for hiding this comment

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

If I wait for the click, the test fails because waitForElementToBeRemoved says that the element was already removed.

You can see this fix in several other tests

Comment on lines -199 to +200
expect(screen.getByText('banana').parentElement).toBeChecked();
expect(screen.getByText('apple').parentElement).not.toBeChecked();
expect(screen.getByRole('radio', {name: 'banana'})).toBeChecked();
expect(screen.getByRole('radio', {name: 'apple'})).not.toBeChecked();
Copy link
Member Author

Choose a reason for hiding this comment

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

the updated eslint rules forbids direct access to RTL elements

await userEvent.click(closeButton);
await waitForElementToBeRemoved(sheet);
}, 30000);
await act(() => userEvent.click(closeButton));
Copy link
Member Author

Choose a reason for hiding this comment

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

🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

why? afaik RTL already wraps these click events with act internally

Copy link
Member Author

@pladaria pladaria Jun 17, 2024

Choose a reason for hiding this comment

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

I'm not sure. Even if the click triggers an async state update, it should be wrapped by the waitForElementToBeRemoved. But without this I was getting missing act errors.

Comment on lines +10 to 13
// @ts-expect-error Chip only accepts string children
<Chip>
{/* @ts-expect-error Chip only accepts string children */}
<div>hello</div>
</Chip>;
Copy link
Member Author

@pladaria pladaria Jun 12, 2024

Choose a reason for hiding this comment

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

this is curious, if I remove the @ts-expect-error the reported issue is:

This JSX tag's children prop expects type string which requires multiple children,
but only a single child was provided.

It works as expected (it only accepts a single string child), but I find the error description misleading

"target": "es2019", // Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019', 'ES2020', or 'ESNEXT'. ,
"target": "es2021", // Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019', 'ES2020', or 'ESNEXT'. ,
Copy link
Member Author

Choose a reason for hiding this comment

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

required to use String.prototype.replaceAll (it is being used in some test)

Comment on lines -47 to +49
{bulletsProps.numPages > 1 && <PageBullets {...bulletsProps} />}
{(bulletsProps.numPages as number) > 1 && (
<PageBullets {...bulletsProps} />
)}
Copy link
Member Author

@pladaria pladaria Jun 12, 2024

Choose a reason for hiding this comment

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

TS v5 now detects (correctly) this as an error, because the type of bulletProps.numPages is:
number | ByBreakpoint<number>

@pladaria pladaria requested review from atabel and marcoskolodny June 12, 2024 11:40
@pladaria pladaria marked this pull request as ready for review June 12, 2024 11:40
src/__tests__/sheet-test.tsx Show resolved Hide resolved
await userEvent.click(closeButton);
await waitForElementToBeRemoved(sheet);
}, 30000);
await act(() => userEvent.click(closeButton));
Copy link
Contributor

Choose a reason for hiding this comment

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

why? afaik RTL already wraps these click events with act internally

yarn.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

please, run a yarn dedupe

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@pladaria pladaria requested a review from atabel June 17, 2024 10:36
@@ -158,9 +158,9 @@ test('Closes a dialog on click outside', async () => {
expect(await screen.findByRole('dialog')).toBeInTheDocument();
expect(await screen.findByRole('button', {name: 'Nope!'})).toBeInTheDocument();

await userEvent.click(screen.getByTestId('dialog-overlay'));
userEvent.click(screen.getByTestId('dialog-overlay'));
Copy link
Contributor

Choose a reason for hiding this comment

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

why this? afaik, userEvent.click() returns a promise: https://testing-library.com/docs/user-event/convenience#click

Copy link
Member Author

@pladaria pladaria Jun 20, 2024

Choose a reason for hiding this comment

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

If I wait for the click action to finish, the dialog element is removed too early and the following waitForElementToBeRemoved fails because the element must exist to wait for removal.

Copy link
Contributor

@yceballost yceballost left a comment

Choose a reason for hiding this comment

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

thx 🫶

@pladaria pladaria enabled auto-merge June 27, 2024 08:34
@pladaria pladaria added this pull request to the merge queue Jun 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 27, 2024
@pladaria pladaria enabled auto-merge July 2, 2024 16:38
@pladaria pladaria added this pull request to the merge queue Jul 2, 2024
Merged via the queue into master with commit 112679f Jul 2, 2024
11 checks passed
@pladaria pladaria deleted the pladaria/WEB-1910_upgrade-deps branch July 2, 2024 16:58
@tuentisre
Copy link
Collaborator

🎉 This PR is included in version 15.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants