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

fix: Remove esModuleInterop from ts-node #7808

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Jun 25, 2020

User facing changelog

This option prevents users from not using it.

Additional details

Why was this change necessary?

This option is too opinionated.

What is affected by this change?

A project will break if it meets these 3 conditions:

  1. no tsconfig.json file
  2. plugin/support files in typescript
  3. use import Foo from 'foo' style

Any implementation details to explain?

N/A

How has the user experience changed?

N/A

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?
  • [N/A] Have API changes been updated in the type definitions?
  • [N/A] Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 25, 2020

Thanks for taking the time to open a PR!

@sainthkh sainthkh added the type: breaking change Requires a new major release version label Jun 25, 2020
@sainthkh sainthkh marked this pull request as ready for review June 25, 2020 03:06
@flotwig flotwig mentioned this pull request Jun 26, 2020
21 tasks
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.

We need a test for this, using a TS project fixture in packages/server and e2e test probably is enough for this

Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

The e2e project here is confusing... it appears we are attempting to test the failure case of an import with esModuleInterop: false right?

It's weird that there are spec files (but don't ever even get run) because the failure happens in the pluginsFile.

I would prefer we actually use a real test failure in a spec file (for instance, asserting that the exported module does or not does have the add function).

It would actually be better to assert in the affirmative and receive a passing test that would otherwise fail with esModuleInterop: true by default.

The test name itself (and comments) could explain why, and reference the original issue. As it stands its very confusing because there's no context as to why the snapshot value of the e2e test is correct or not.

@sainthkh
Copy link
Contributor Author

It seems that there is no alternative.

1. About using spec files.

As we know, spec files and plugin/support files use different TypeScript transpiler settings. As for spec files, using opinionated typescript setting is necessary because tsify doesn't have transpileOnly option and that's the only way not to lose the traspile speed.

Because of that, using spec files cannot test if esModuleInterop can be overridden for support/plugin files in Cypress.

I explained this in the plugin file and removed unnecessary tests in spec files.

2. Creating affirmative test.

I found out that it's impossible to create an affirmative test with math.add. Because:

import esModuleInterop Result
import * as math from './math' false pass
import * as math from './math' true pass
import math from './math' false fail
import math from './math' true pass

I changed it to simply export add function.

@robcresswell
Copy link

Hello! Is this likely to land in 5.0? See to have stalled for a few weeks. Is there anything I can do to help?

@jennifer-shehane
Copy link
Member

@robcresswell We're still reviewing the final 5.0 features. Hopefully this gets in. #7753

@robcresswell
Copy link

Ah sorry, thats poor research on my part. Thanks for the link @jennifer-shehane!

@jennifer-shehane
Copy link
Member

@sainthkh Will this require any notes on our Migration Guide? Any updates people will need to be aware of for changes they need to make? I just noticed this is the only 5.0 PR that doesn't have specific docs updates.

@sainthkh
Copy link
Contributor Author

When a user doesn't have a tsconfig.json file or define esModuleInterop: true, Cypress didn't fail in 4.x, but it might fail after 5.0. Because Cypress now uses the default esModuleInterop setting of TypeScript.

I used 'might', because it wouldn't fail when a user didn't use import syntax.

@chrisbreiding chrisbreiding changed the base branch from v5.0-release to tr-132-change-esmodule-interop-default August 3, 2020 13:32
@chrisbreiding chrisbreiding changed the title Remove esModuleInterop from ts-node fix: Remove esModuleInterop from ts-node Aug 3, 2020
@chrisbreiding
Copy link
Contributor

Merging this into #8143 and continuing any work there.

@chrisbreiding chrisbreiding merged commit 55e97a1 into cypress-io:tr-132-change-esmodule-interop-default Aug 3, 2020
brian-mann added a commit that referenced this pull request Aug 10, 2020
…sor (#8143)

* make tests preprocessor agnostic

* update eslintignore

* put back deps needed for e2e test

* remove obselete snapshot

it was replaced by 1_typescript_support_spec.ts.js

* switch from browserify to webpack preprocessor

* cmon github

* fix/update tests

* bump preprocessor and update snapshots

* update snapshots

* bump preprocessor to gain json support

* fix e2e tests with webpack-originated errors

* bump preprocessor version, fix node globals

* update snapshot

* remove support for ? in file path

* bump preprocessor version

* bump preprocessor again

* bump preprocessor

* bump preprocessor

* update snapshots

* bump preprocessor version

* bump preprocessor, quiet the paths plugin

* add test verifying tsconfig paths work

* refactor registering ts-node

* separate spec/support file typescript tests from plugins file typescript tests

* fix unit tests

* fix: Remove esModuleInterop default from ts-node (#7808)

* Remove esModuleInterop from ts-node.

* Add e2e test.

* Fix test.
Change test name.
Add comment.

* Fix test snapshot name.

* update snapshotting

Co-authored-by: Chris Breiding <chrisbreiding@gmail.com>

* clean up e2e project

* bump preprocessor to 1.3.2, which removes esModuleInterop default value

* improve esmoduleinterop e2e tests

* change spec file

* bump batteries-included preprocessor and install latest webpack preprocessor beside it

* update snapshots

* put back snapshot

* update snapshot

* update snapshot

Co-authored-by: Kukhyeon Heo <sainthkh@naver.com>
Co-authored-by: Brian Mann <brian.mann86@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: breaking change Requires a new major release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants