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: specify TS 5.x support #26413

Merged
merged 3 commits into from
Apr 6, 2023
Merged

chore: specify TS 5.x support #26413

merged 3 commits into from
Apr 6, 2023

Conversation

lmiller1990
Copy link
Contributor

Additional details

Specify Cypress CT works with TypeScript 5.x.

Steps to test

You could make a project using CT and TS 5.x There's zillions of permutations (Next, Vue, Angular...). No doubt some specific combinations might not work, but in general, none of the breaking changes impact us from what I can see.

How has the user experience changed?

Now when setting up CT, users with TS 5.x installed won't feel like it's not supported when going through the onboarding workflow, which is currently the case: #26204. This makes for a bad onboarding and first 5 minutes experience, which is essential when adopting a new tool.

PR Tasks

@lmiller1990 lmiller1990 requested a review from a team April 4, 2023 00:50
@cypress
Copy link

cypress bot commented Apr 4, 2023

4 flaky tests on run #45305 ↗︎

0 5220 74 0 Flakiness 4

Details:

separate project
Project: cypress Commit: 0b017850b3
Status: Passed Duration: 17:35 💡
Started: Apr 6, 2023 12:47 AM Ended: Apr 6, 2023 1:05 AM
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron

View Output Video

Test Artifacts
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video
Flakiness  next.cy.ts • 1 flaky test • webpack-dev-server

View Output Video

Test Artifacts
Working with next-12.1.6 > should show compilation errors on src changes Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Contributor

@mike-plummer mike-plummer left a comment

Choose a reason for hiding this comment

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

Nice! Noticed one thing with the existing version range on TS but not strictly related to this issue - feel free to adjudicate

@@ -49,7 +49,7 @@ export const WIZARD_DEPENDENCY_TYPESCRIPT = {
package: 'typescript',
installer: 'typescript',
description: 'TypeScript is a language for application-scale JavaScript',
minVersion: '^=3.0.0 || ^=4.0.0',
minVersion: '^=3.0.0 || ^=4.0.0' || '^=5.0.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a disconnect between the docs and our min TS version here - should we update to 3.4.0 while we're in here?

Copy link
Contributor Author

@lmiller1990 lmiller1990 Apr 5, 2023

Choose a reason for hiding this comment

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

I think the docs 3.4 limitation is specific to E2E - maybe something to do with the usage of ts-loader... I have no idea why we need 3.4 or if it's applicable here.

3.4: https://devblogs.microsoft.com/typescript/announcing-typescript-3-4/

Copy link
Contributor

@warrensplayer warrensplayer left a comment

Choose a reason for hiding this comment

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

I think the update to support TS 5 is fine, but I have a question about the tests.

@lmiller1990 Why did you replace the TS 4.5 test with a TS 5 test? It seems like the original TS 4.5 test was there to test that the preserveValueImports parameter that was introduced in TS 4.5 does not break earlier versions. It was introduced here: #20889

Shouldn't we leave this test and just add a new system test for TS 5?

@lmiller1990
Copy link
Contributor Author

@warrensplayer I might have misunderstood the original test, I thought the idea was "make sure it works with TS 4.5+". I was just trying to minimize the number of projects - this should still capture the original intent... but I can make a separate project, that might be more maintainable long term. What do you think?

@warrensplayer
Copy link
Contributor

@lmiller1990 Just commenting my Slack conversation here:

I see your point about not wanting more fixtures.
The original test for 4.5 was released with the code here which specifically looks for TS 4.5 or greater. My thought at the time was that someone could change that code to say TS 5 or greater and the tests would pass, but TS 4.5 - < TS 5 would be broken.

Not going to block on this. I will approve.

@lmiller1990
Copy link
Contributor Author

@warrensplayer made a separate test project.
@mike-plummer I pushed a commit clarifying TS support at ^=3.4.0 to be the same as E2E.

@lmiller1990 lmiller1990 merged commit 80fcb8b into develop Apr 6, 2023
@lmiller1990 lmiller1990 deleted the issue-26204 branch April 6, 2023 03:09
astone123 pushed a commit to kgroat/cypress that referenced this pull request Apr 19, 2023
* chore: specify TS 5.x support

* add test

* separate project
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.

Component Testing: typescript ^=5.0.0 support
3 participants