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

[TS 4.8] Deprecated factory methods aren't usable with originalNode.modifiers #49939

Closed
frigus02 opened this issue Jul 18, 2022 · 4 comments
Closed
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status.

Comments

@frigus02
Copy link
Contributor

Bug Report

Deprecated factory methods aren't usable with originalNode.modifiers.

#49089 changed Node.prototype.modifiers from NodeArray<Modifier> to NodeArray<Modifier | Decorator>. It also changed factory functions to accept a single NodeArray<Modifier | Decorator> argument instead of separate modifiers and decorators arguments. The old factory functions are still available (they are deprecated now). But I imagine most usages of those old functions now result in a compile error for passing in originalNode.modifiers:

Argument of type 'NodeArray' is not assignable to parameter of type 'readonly Modifier[]'

🔎 Search Terms

4.8, modifiers, ModifiersArray, ModifierLike, decorators, Modifier, Decorator, factory

🕗 Version & Regression Information

  • This changed between versions 4.7.4 and 4.8-beta

⏯ Playground Link

Playground link with relevant code

Note: Not sure how to reproduce this in the playground. import from "typescript" doesn't seem to import nightly typings in the playground.

💻 Code

import * as ts from "typescript";

function transformClassDecl(orig: ts.ClassDeclaration) {
    return ts.factory.updateClassDeclaration(
        orig,
        orig.decorators,
        orig.modifiers,
//      ^^^^^^^^^^^^^^ TS2345: Argument of type 'NodeArray<ModifierLike>' is not assignable to parameter of type 'readonly Modifier[]'.
        orig.name,
        orig.typeParameters,
        orig.heritageClauses,
        orig.members);
}

🙁 Actual behavior

This code snippet fails to compile with the error message inlined in the snippet.

🙂 Expected behavior

Ideally the deprecated factory.updateXxx functions would still be usable as before. This would make the upgrade to TypeScript 4.8 easier.

I imagine most usages pass in originalNode.modifiers unchanged, unless they modify the modifiers. Picking Angular as a random example:

https://github.com/angular/angular/blob/557cf7dc63fe768900f296252e1a96e6166b24df/packages/compiler-cli/src/ngtsc/transform/src/transform.ts#L210

Workarounds

We can change usages of factory functions to make code compatible with both TypeScript 4.7 and 4.8 by doing something like:

ts.factory.updateClassDeclaration(
  orig,
  orig.decorators,
  orig.modifiers?.filter(ts.isModifier),
  ...
)
@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jul 29, 2022
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.8.1 milestone Jul 29, 2022
@rbuckton
Copy link
Member

rbuckton commented Aug 19, 2022

Unfortunately, there's only so much we can do for backwards compatibility here. If we were to allow ModifierLike[] for the modifiers parameter, we would have to somehow differentiate between decorators supplied via the decorators parameter and existing decorators that may be passed via modifiers. However, there's no way to mitigate that kind of collision.

If you need to support both TS 4.7 and TS 4.8, my recommendation would be to do as you have described in the workaround, or to branch based on ts.version or ts.versionMajorMinor. Note that #50343 will likely further break compatibility here in favor of producing expected build breaks when accessing decorators, though TS 4.8 exports several functions that can help to access only the decorators or modifiers of a node:

  • ts.canHaveModifiers(node)
  • ts.canHaveDecorators(node)
  • ts.getModifiers(node)
  • ts.getDecorators(node)

To bridge between 4.7 and 4.8, I would recommend you have something like the following in your code:

function tryGetDecorators(ts: typeof import("typescript"), node: Node) {
  if (typeof ts.getDecorators === "function") {
    // TS 4.8 and later
    return ts.canHaveDecorators(node) ? ts.getDecorators(node) : undefined;
  }

  // TS 4.7 and earlier
  return node.decorators;
}

function tryGetModifiers(ts: typeof import("typescript"), node: Node) {
  if (typeof ts.getModifiers === "function") {
    // TS 4.8 and later
    return ts.canHaveModifiers(node) ? ts.getModifiers(node) : undefined;
  }

  // TS 4.7 and earlier
  // NOTE: Returns original array if every node was a `Modifier`, since the `update` methods on `NodeFactory`
  // use reference equality.
  return ts.visitNodes(node.modifiers, node => ts.isModifier(node) ? node : undefined, ts.isModifier);
}

@frigus02
Copy link
Contributor Author

frigus02 commented Aug 22, 2022

Thanks for looking into this. I was indeed hoping you could allow ModifierLike[] for the modifiers parameter. But your explanation makes sense.

We're going to use the workaround and some of the helpers you suggested.

I think this issue can be closed.

@frigus02
Copy link
Contributor Author

@rbuckton I just noticed that canHaveModifiers and canHaveDecorators are @internal. Are they supposed to be public?

@rbuckton
Copy link
Member

@rbuckton I just noticed that canHaveModifiers and canHaveDecorators are @internal. Are they supposed to be public?

Ah, yes they are. #50399 should fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

No branches or pull requests

3 participants