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

Opinionated TS transpilation without overrides is problematic #7575

Closed
robcresswell opened this issue Jun 3, 2020 · 11 comments
Closed

Opinionated TS transpilation without overrides is problematic #7575

robcresswell opened this issue Jun 3, 2020 · 11 comments
Assignees
Labels
topic: typescript v4.6.0 🐛 Issue present since 4.6.0

Comments

@robcresswell
Copy link

robcresswell commented Jun 3, 2020

Current behavior:

Cypress recently added better native support for TypeScript, a great step forward, thanks!

However, part of this process added the following defaults for ts-node:

Specifically, esModuleInterop is problematic. This is an opinionated transpilation, and cannot be overridden. Typically when working working with a JS / TS mix, you can either use imports such as

import * as Foo from 'foo';

or you can enable esModuleInterop and do

import Foo from 'foo';

In my case, I chose the former, to reduce the "magic" that is done by the TypeScript compiler; however, because Cypress enforces the latter, I get lots of errors along the lines of

TypeError: Foo is not a function

Desired behavior:

A test frameworks authors should not enforce an opinion about how I build my application; this esModuleInterop value should be possible to override. I tried manually changing it by editing the downloaded Cypress JS code in ~/.cache, and the tests ran as expected.

Test code to reproduce

Working on a minimal reproduction, please give me a bit of time 😄

Versions

Cypress: 4.7.0

@jennifer-shehane
Copy link
Member

This change was made in 4.6.0 as part of #7197 so perhaps @bahmutov can speak to why this decision was made.

@jennifer-shehane jennifer-shehane added the v4.6.0 🐛 Issue present since 4.6.0 label Jun 3, 2020
@cypress-bot cypress-bot bot added the stage: proposal 💡 No work has been done of this issue label Jun 3, 2020
@bahmutov
Copy link
Contributor

bahmutov commented Jun 3, 2020

I did move it, but this option was already there https://github.com/cypress-io/cypress/pull/7197/files#diff-0f01b0186fbd767d98bd689a60ddd05aL193

I do wonder if we really need it or not, and if we should overwrite it from the user's TS config

@robcresswell
Copy link
Author

I feel quite strongly that enforcing this is a mistake on Cypress' part, though I can see why it was done. Haven't quite had time yet to put up a minimal repro, though it seems that my intent is at least understandable without it, for now.

Is this something I should put up a PR for? I don't know how accessible this is a Cypress newcomer, though I can see you've got a lot going on 😅 and I'm happy to give it a go.

@bahmutov
Copy link
Contributor

bahmutov commented Jun 4, 2020

@sainthkh do you remember why you have set esModuleInterop: true? Was there a specific reason to override user's setting?

@sainthkh
Copy link
Contributor

sainthkh commented Jun 5, 2020

It was mainly because I misunderstood the default behavior of tsnode. I thought it would load tsconfig.json from Cypress internal ones. But I was wrong.

Maybe I was more familiar with import Foo from 'foo' and felt it was a bit weird that we cannot use it without tsconfig.json. It seems that I did too much.

But removing it would break some test setups with no tsconfig.json + ts plugin/support files + import Foo from 'foo'-style import. It's a bug, not a breaking change. Right?

@robcresswell
Copy link
Author

@bahmutov @sainthkh Any thoughts on fixing this?

@sainthkh
Copy link
Contributor

The only block for me is to decide if it's a breaking change or a bug. If it's a bug, it can be fixed immediately. If it's a breaking change, we need to wait for 5.0.

@cypress-bot cypress-bot bot added stage: work in progress stage: needs review The PR code is done & tested, needs review and removed stage: proposal 💡 No work has been done of this issue stage: work in progress stage: needs review The PR code is done & tested, needs review labels Jun 25, 2020
@cypress-bot cypress-bot bot added stage: needs review The PR code is done & tested, needs review and removed stage: work in progress labels Jul 6, 2020
@chrisbreiding
Copy link
Contributor

@robcresswell Can you provided a reproducible example of this? We're looking into changing this default for 5.0, but I can't create a case where import * as Foo from 'foo' fails.

@robcresswell
Copy link
Author

@chrisbreiding I've pushed an example to https://github.com/robcresswell/cypress-example

Let me know if you have any difficulties; there's an explanation in the README.

@chrisbreiding chrisbreiding added stage: needs review The PR code is done & tested, needs review and removed stage: pending release labels Aug 3, 2020
@cypress-bot cypress-bot bot added stage: work in progress stage: needs review The PR code is done & tested, needs review and removed stage: needs review The PR code is done & tested, needs review stage: work in progress labels Aug 7, 2020
@sync-by-unito sync-by-unito bot closed this as completed Aug 10, 2020
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 10, 2020

The code for this is done in cypress-io/cypress#8143, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot cypress-bot bot removed the stage: needs review The PR code is done & tested, needs review label Aug 10, 2020
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 20, 2020

Released in 5.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v5.0.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
topic: typescript v4.6.0 🐛 Issue present since 4.6.0
Projects
None yet
Development

No branches or pull requests

5 participants