fix: remove some CT functions and props #24419
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
||||||||||||||||||||||||||||||
…press into astone123/ct-functionality
| @@ -47,6 +47,6 @@ export function mount (jsx: React.ReactNode, options: MountOptions = {}, rerende | |||
| return makeMountFn('mount', jsx, { ReactDom: ReactDOM, ...options }, rerenderKey, internalOptions) | |||
| } | |||
|
|
|||
| export function unmount (options = { log: true }) { | |||
There was a problem hiding this comment.
This is a dead-end, we should still export it but throw an error in the exported version.
There was a problem hiding this comment.
Should we deprecate and warn -> remove in v12? Just to make migration a bit smoother.
There was a problem hiding this comment.
I think marking as deprecated as Lachlan suggests makes sense. As far as erroring or warning, my vote is to stay with erroring based on the assumption that someone calling unmount is either wanting to test the "unmount" behavior or perform some sort of cleanup. Not erroring would most likely cause some sort of other error or problem that would be harder to track down. I could be swayed otherwise though.
There was a problem hiding this comment.
Fine with a straight up deprecation and error if we have a obvious alternative/migration strategy - a straight up deprecation without a recommendation is like "your test is broken now, good luck fixing it" which is not a nice DX.
There was a problem hiding this comment.
@ZachJW34 @astone123 @warrensplayer I thought about this a bit more. Here's my recommendations. I added some commits to make sure everything works (seems fine).
- Export this, but error. The error contains a link to the migration guide. Added here.
- Have a non exported
unmount(I called itinternalUnmount) we use for unmounting the component. - Provide a migration strategy. I added an example showing how to unmount without
unmounthere and in my migration docs.
| @@ -47,6 +47,6 @@ export function mount (jsx: React.ReactNode, options: MountOptions = {}, rerende | |||
| return makeMountFn('mount', jsx, { ReactDom: ReactDOM, ...options }, rerenderKey, internalOptions) | |||
| } | |||
|
|
|||
| export function unmount (options = { log: true }) { | |||
There was a problem hiding this comment.
Should we deprecate and warn -> remove in v12? Just to make migration a bit smoother.
npm/react/docs/styles.md
Outdated
| @@ -41,6 +41,25 @@ it('is stylish', () => { | |||
| }) | |||
| ``` | |||
|
|
|||
| ## Import from support file | |||
|
|
|||
| If you have stylesheets that should apply to all of your components, you can import those from your component support file. | |||
There was a problem hiding this comment.
| If you have stylesheets that should apply to all of your components, you can import those from your component support file. | |
| If you have stylesheets that should apply to all of your components, you can import those in your component support file. |
|
|
||
| // pause to show it | ||
| cy.wait(1000) | ||
| cy.get('.square').click() | ||
| cy.wait(1000) | ||
|
|
||
| // check if style was applied |
There was a problem hiding this comment.
Could probably leave this comment.
| @@ -1,21 +0,0 @@ | |||
| # unmount | |||
There was a problem hiding this comment.
There are other tests that were changed to remove the unmount command and replace it with mounting a new test component instead to cause the original mounted component to unmount. Should we keep this unmount directory here and rewrite the examples to explicitly show how to test unmount functionality in a component or do those other examples suffice?
There was a problem hiding this comment.
Yes, agreed, I was thinking this same thing, I'm going to revive this directory and explain the migration path.
| @@ -47,6 +47,6 @@ export function mount (jsx: React.ReactNode, options: MountOptions = {}, rerende | |||
| return makeMountFn('mount', jsx, { ReactDom: ReactDOM, ...options }, rerenderKey, internalOptions) | |||
| } | |||
|
|
|||
| export function unmount (options = { log: true }) { | |||
There was a problem hiding this comment.
I think marking as deprecated as Lachlan suggests makes sense. As far as erroring or warning, my vote is to stay with erroring based on the assumption that someone calling unmount is either wanting to test the "unmount" behavior or perform some sort of cleanup. Not erroring would most likely cause some sort of other error or problem that would be harder to track down. I could be swayed otherwise though.
|
Okay, pushed two commits.
I also wrote a list of deprecations and how to migrate, we should publish this somewhere and link to it as part of the error messages.
Errors now link to migration guide. This is a better DX - it gives the user something actionable. |
|
On links: 6e98295 Consolidated in the same place in the same fashion as E2E does it. |
|
Note: on links appear like this (note they have a "Learn More" link pointing to
|
npm/react/cypress/component/advanced/set-timeout-example/loading-indicator.cy.jsx
Show resolved
Hide resolved
| @@ -1,4 +1,4 @@ | |||
| import { JSX } from './createMount' | |||
| type JSX = Function & { displayName: string } | |||
There was a problem hiding this comment.
Just out of curiosity: What is the reason we define it this way?
…stone123/ct-functionality
lmiller1990
force-pushed
the
astone123/ct-functionality
branch
from
abde2e4
to
bc9d8cd
Compare
User facing changelog
mounthave been removedcypress/reactmountHookfunction no longer existscypress/reactunmountfunction is no longer exportedcypress/vueandcypress/vue2no longer exportmountCallback. Usemountdirectlycypress/reactno longer exportsmountas a default export. Use destructuring:import { mount } from 'cypress/react'.Additional details
For GA we're taking the opportunity to make breaking changes to clean things up and remove functionality that we don't need anymore. This PR does the following:
mountmountHookand related examples and tests fromcypress/reactunmountinternal only for React - no longer exported for use outside of thecypress/reactpackagemountCallbackfromcypress/vueandcypress/vue2mountshould be used directly insteadNOTE: There are a bunch of example tests in
npm/vuethat we're not running in CI, and I can't get them to pass. I decided to leave all of those as they were (some are still injecting styles and/or usingmountCallbackwhich is now invalid as of this PR). I created an issue to handle these tests here #24423Steps to test
I tried to break these up into small commits since there are a fair amount of changes here. It starts pretty clean in the top commits but towards the end things get a little more messy once I realized that I had broken some tests.
A lot of these are documentation changes so there's not much to test there, but it would be good to review the Percy snapshot changes and of course, the code changes.
PR Tasks
cypress-documentation? I looked through the docs and I didn't see anything that needed to be updated based on these changestype definitions?