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

Out-of-the-box TypeScript Support #5906

Merged
merged 31 commits into from
Apr 13, 2020

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Dec 7, 2019

User facing changelog

You don't need to install TypeScript plugin to use Cypress. It works out-of-the-box.

Additional details

Why was this change necessary?

Many developers are now using TypeScript and want to use Cypress + TypeScript without any configuration.

What is affected by this change?

Bundle size will become a bit bigger because ts-node. (If it's a problem, we need to drop TypeScript support for plugins.)

Any implementation details to explain?

I've developed a small transform that transpiles the ts files.

How has the user experience changed?

  • Before: Install TypeScript plugin. You cannot use ts in plugins.
  • After: Just drop ts files. It works.

PR Tasks

  • Have tests been added/updated?
  • Has the original issue been tagged with a release in ZenHub?
  • Has a PR for user-facing changes been opened in cypress-documentation?
    • TypeScript related documents should be changed.
  • [NA] Have API changes been updated in the type definitions?
  • [NA] Have new configuration options been added to the cypress.schema.json? -> No options added.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 7, 2019

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

  • Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
  • Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

If any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'.

User Experience

  • The feature/bugfix is self-documenting from within the product.
  • The change provides the end user with a way to fix their problem (no dead ends).

Functionality

  • The code works and performs its intended function with the correct logic.
  • Performance has been factored in (for example, the code cleans up after itself to not cause memory leaks).
  • The code guards against edge cases and invalid input and has tests to cover it.

Maintainability

  • The code is readable (too many nested 'if's are a bad sign).
  • Names used for variables, methods, etc, clearly describe their function.
  • The code is easy to understood and there are relevant comments explaining.
  • New algorithms are documented in the code with link(s) to external docs (flowcharts, w3c, chrome, firefox).
  • There are comments containing link(s) to the addressed issue (in tests and code).

Quality

  • The change does not reimplement code.
  • There's not a module from the ecosystem that should be used instead.
  • There is no redundant or duplicate code.
  • There are no irrelevant comments left in the code.
  • Tests are testing the code’s intended functionality in the best way possible.

Internal

  • The original issue has been tagged with a release in ZenHub.

@sainthkh
Copy link
Contributor Author

sainthkh commented Dec 7, 2019

I don't know why but stack trace message structures are a bit changed. So, updated snapshots.

@sainthkh sainthkh marked this pull request as ready for review December 9, 2019 01:11
@sainthkh sainthkh changed the title WIP Out-of-the-box TypeScript Support Out-of-the-box TypeScript Support Dec 9, 2019
@sainthkh sainthkh force-pushed the issue-1859-part-1 branch 2 times, most recently from 25ee491 to aae6ce1 Compare December 18, 2019 02:50
@sainthkh sainthkh changed the title Out-of-the-box TypeScript Support WIP Out-of-the-box TypeScript Support Dec 18, 2019
@sainthkh sainthkh changed the title WIP Out-of-the-box TypeScript Support Out-of-the-box TypeScript Support Dec 26, 2019
@jennifer-shehane jennifer-shehane requested a review from a team January 9, 2020 06:22
@jennifer-shehane

This comment has been minimized.

@sainthkh
Copy link
Contributor Author

@jennifer-shehane It passed build this time.

@bahmutov
Copy link
Contributor

bahmutov commented Feb 3, 2020

I have tested this PR locally against a new project, this is pretty incredible.

  1. Scaffold a new project (it outputs JavaScript) and rename a spec to .ts and it just works

Screen Shot 2020-02-03 at 12 24 14 PM

  1. Create new TS spec and it works

Screen Shot 2020-02-03 at 12 25 16 PM

If you rename plugins or support file and restart the test runner, then it works, nice.

But if you rename the plugins file to plugins/index.ts or the support file to support/index.ts, then it fails (need to restart the test runner, but the error message does not say so)

Screen Shot 2020-02-03 at 12 26 05 PM

I am fine with including ts-node as a dependency, I think increased size is worth the benefits - a lot of users love TypeScript and would love native support in Cypress

Copy link
Contributor

@bahmutov bahmutov left a comment

Choose a reason for hiding this comment

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

I love the functionality, I just think that if you rename plugins/index.js to plugins/index.ts while the test runner is running it should intelligently handle this.

Same for support file.

@sainthkh sainthkh changed the title Out-of-the-box TypeScript Support [WIP] Out-of-the-box TypeScript Support Feb 21, 2020
@sainthkh sainthkh force-pushed the issue-1859-part-1 branch 3 times, most recently from f7ed97b to d486202 Compare March 24, 2020 08:07
@sainthkh sainthkh changed the title [WIP] Out-of-the-box TypeScript Support Out-of-the-box TypeScript Support Mar 24, 2020
@sainthkh sainthkh requested a review from bahmutov March 24, 2020 08:54
@sainthkh
Copy link
Contributor Author

  • Rebased and fixed failures.
  • I didn't do anything but appveyor fails. What happened?

@bahmutov
Copy link
Contributor

Nice @sainthkh I will fix the AppVeyor - one of my collaborators on print-env made a new v2 release, which has a breaking change https://github.com/bahmutov/print-env/releases so I need to update how it is used here

this.push(text)
} else {
this.push(ts.transpileModule(text, {
compilerOptions: {
Copy link
Contributor

@CypressJoseph CypressJoseph Mar 25, 2020

Choose a reason for hiding this comment

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

so this completely overrides specific configuration in tsconfig.json, if i understand?

Copy link
Contributor

Choose a reason for hiding this comment

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

(that seems reasonable honestly, just thinking about it)

Copy link
Contributor Author

@sainthkh sainthkh Mar 26, 2020

Choose a reason for hiding this comment

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

Yes, it does. Because for the speed, it ignores the type check. If we do type check, it takes about 7-10 sec to load a test file.

CypressJoseph
CypressJoseph previously approved these changes Mar 25, 2020
Copy link
Contributor

@CypressJoseph CypressJoseph left a comment

Choose a reason for hiding this comment

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

this looks great!! i think i do wonder about: how users can extend the configuration we use to transpile, if they might need to.

this also makes me wonder whether we are emitting ts instead of js for examples on first install? if we're not maybe this unlocks a good opportunity to do that (we'd have to author examples in TS ofc)

@sainthkh sainthkh force-pushed the issue-1859-part-1 branch from 9ad54d6 to 3512f4d Compare April 13, 2020 07:43
@sainthkh
Copy link
Contributor Author

  • Rebased and fixed all.
  • Flaky test.

@chrisbreiding
Copy link
Contributor

This looks really good.

I think the only thing left to do is update the documentation and recipes regarding TypeScript.

  • The TypeScript doc should now make it clear TypeScript is supported out of the box. We don't really need to reference the recipes for setting up a preprocessor anymore. Instead, we should point out they need to have their own typescript package installed for it to work.
  • We should update the recipes that involve TypeScript. Maybe remove everything but their READMEs and point out that it's included out of the box now.

@chrisbreiding
Copy link
Contributor

Gleb made a good point that users could be on an older version of Cypress, so we should keep the recipes around, just add a note that says if you're in version x.x.x or later, it's supported by default.

We don't need a similar note in the docs, since there's an underlying assumption with the docs that it relates to the latest version of Cypress.

@bahmutov
Copy link
Contributor

bahmutov commented Apr 13, 2020 via email

@jennifer-shehane
Copy link
Member

Need to update the docs with this, but going to merge this in to try to get it into our release today and see how our tests run against it in our example projects in develop. So I'll open a PR for the docs.

@kpittman-securus
Copy link

@jennifer-shehane Awesome! Do we know what version number this will be released with? Our team is looking at updating our version of Cypress and I'd love for this feature to be included.

@Saibamen
Copy link
Contributor

@cypress-io cypress-io locked as resolved and limited conversation to collaborators Apr 14, 2020
@@ -99,6 +101,23 @@ class Project extends EE {

return scaffold.plugins(path.dirname(cfg.pluginsFile), cfg)
}
}).then((cfg) => {
try {
const tsPath = resolve.sync('typescript', {
Copy link
Member

Choose a reason for hiding this comment

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

this PR has already been merged, but it is a bad idea to ever use sync functions which read from the file system. this should use the async variant that does async fs calls under the hood. this avoids potential EMFILE errors.

@@ -173,9 +177,38 @@ module.exports = (ipc, pluginsFile) => {
return false
})

if (!tsRegistered) {
try {
const tsPath = resolve.sync('typescript', {
Copy link
Member

Choose a reason for hiding this comment

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

see other comment about EMFILE

}

try {
return resolve.sync('typescript', {
Copy link
Member

Choose a reason for hiding this comment

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

see other comment about EMFILE

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

Successfully merging this pull request may close these issues.

More Emphasis on Typescript Proposal: add native built-in typescript support
8 participants