Skip to content

Fix super method call chain, remove ? from method, improve vary-by #35013

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

Merged
merged 2 commits into from
Nov 19, 2019

Conversation

rbuckton
Copy link
Contributor

@rbuckton rbuckton commented Nov 9, 2019

This fixes our emit for super.method?.() and super["method"]?.(), as well as an incorrect emit for class Base { method?() {} }.

This also improves our ability to write compiler/conformance tests with multiple variations to now support:

  • * to include all possible values for an option (i.e. // @target: *)
  • A - or ! prefix to exclude a value from an option (i.e. // @target: *,-es2015)
  • Expanded the list of vary-by settings to include a number of additional settings (i.e. // @strict: *). The full list is in src\testRunner\compilerRunner.ts
  • Limits the maximum number of variations to 25 so that we don't end up tests that try to generate combinations of every possible option.

Fixes #34952

At this point, this primarily just adds some additional tests and the improvements to vary-by.

@rbuckton
Copy link
Contributor Author

rbuckton commented Nov 9, 2019

I just noticed that #34951 may also fix the super.method?.() part, I'll revise this PR as necessary shortly.

@@ -3593,6 +3593,7 @@ namespace ts {
// overloads are TypeScript syntax.
if (node.decorators
|| hasModifier(node, ModifierFlags.TypeScriptModifier)
|| node.questionToken
Copy link
Member

Choose a reason for hiding this comment

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

Already fixed in @ajafff's PR, but we should keep your tests.

@@ -0,0 +1,8 @@
// @target: esnext,es2016
Copy link
Member

Choose a reason for hiding this comment

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

isn't the ES5 emit interesting here too? I assume we could be putting a ? there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ES5 emit changes the entire declaration to a function expression, so it wouldn't be caught by this change.

# Conflicts:
#	src/compiler/transformers/esnext.ts
@rbuckton rbuckton merged commit 6c59dc3 into master Nov 19, 2019
@rbuckton rbuckton deleted the fix34952 branch November 19, 2019 18:11
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.

Optional Chaining downlevel emit is wrong for super.method?()
2 participants