"The field is already decorated" errors in derived classes after mobx-undecorate

[Gofilord]

Intended outcome:

Update to Mobx6, run the mobx-undecorate tool with --keepDecorators flag (because the codebase is huge), and everything works. Codemod recognizes derived classes and removes decorators where appropriate.

Actual outcome:

Updated and ran the codemod. All derived classes that override methods and apply a mobx decorator to them generate an error such as:

How to reproduce the issue:
Here is a CodeSandbox that recreates a single example AFTER the codemod was ran:
https://codesandbox.io/s/modest-rui-4s467d?file=/index.js

Obviously, removing the decoration at the derived class solves the issue, but my code base is huge and I can't possibly go over each case like this and by hand remove the decorators.

Versions
I have went over each and every step of the migration guide, including the changes to the tsconfig and babelrc. The issue still reproduces.

[urugator]

These tools usually process one file at a time and they don't know how to resolve imports, so it's quite hard to detect whether a member is present in super class. I think we could at least rely on the presence of TS override keyword, PR welcome.

[Gofilord]

@urugator Thank you for the quick response. Can you please elaborate on the solution?

[urugator]

Can you please elaborate on the solution?

The assumption is that you use TS with --noImplicitOverride, then the idea is to change:

mobx/packages/mobx-undecorate/src/undecorate.ts

Lines 390 to 397 in 5e6584b

	if (options?.keepDecorators !== true) property.decorators.splice(0)
	effects.membersMap.push([
	property.key,
	expr,
	property.computed,
	property.accessibility === "private" || property.accessibility === "protected"
	])

into something like

if (options?.keepDecorators !== true) property.decorators.splice(0)

// Replace decorator with @override
if (property.override && options?.keepDecorators) {
  const overrideDecorator = /* TODO assemble override decorator + add import */ 
  property.decorators[0] = overrideDecorator; 
  expr = overrideDecorator.expression;
}

effects.membersMap.push([
  property.key,
  expr,
  property.computed,
  property.accessibility === "private" || property.accessibility === "protected"
])

However babel/parser doesn't seem to support override keyword, so we would also have to change parser to @typescript-eslint/parser.

[Gofilord]

I am not familiar enough with codemods, but it seems like it doesn't go into the condition body because property.override is not defined. I tried replacing the babel/parser with @typescript-eslint/parser and it resulted in this error:

Transformation error (did not recognize object of type "PropertyDefinition"

There must be a lot of users that want to upgrade mobx in their large codebases and would benefit from this fix.
Is it possible one of the maintainers do it? I will continue trying none the less.

[mweststrate]

Could it be that the babel parser might just need a bump to support override annotation?

…

On Tue, 12 Jul 2022, 10:58 Oren, ***@***.***> wrote: I am not familiar enough with codemods, but it seems like it doesn't go into the condition body because property.override is not defined. I tried replacing the babel/parser with @typescript-eslint/parser and it resulted in this error: Transformation error (did not recognize object of type "PropertyDefinition" There must be a lot of users that want to upgrade mobx in their large codebases and would benefit from this fix. Is it possible one of the maintainers do it? I will continue trying none the less. — Reply to this email directly, view it on GitHub <#3460 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBF4RDEBSYQ25C256BDVTU6VPANCNFSM53HXDV6A> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Gofilord]

@mweststrate Using the latest babel parser version, if that's what you meant.
Maybe a different approach is the codemod will save a mapping of classes and methods it acted upon, and when it comes across a class that extends another class it could check against that mapping if a method was overridden and remove the decorator?

[urugator]

Just some notes:

benjamn/ast-types#728

https://github.com/benjamn/ast-types#custom-ast-node-types

export const parser = "ts" should expose override as MethodDeclaration.modifiers[OverrideKeyword]

EDIT:
benjamn/recast#1151

[urugator]

Omg obviously we are using old babel parser, because undecorate was missing @babel/parser dependency and jscodeshift version has nothing to do with parser version...

[urugator]

#3478

[Gofilord]

@urugator This looks like the fix is only if my codebase uses the override keyword, right?

[urugator]

Yes, but I kinda hope there is some --noImplicitOverride autofix available... obviously it also requires TS.

[mweststrate]

Sounds like a potential idea, the two problems I can imagine with that is that a) a class declaration name doesn't guarantee that it is used under the same name in extends clause, b) there might be multiple classes with the same name. c) it only works in the lucky case the superclass file is processed before the subclass file.

…

On Wed, Jul 13, 2022 at 8:04 AM Oren ***@***.***> wrote: @mweststrate <https://github.com/mweststrate> Using the latest babel parser version, if that's what you meant. Maybe a different approach is the codemod will save a mapping of classes and methods it acted upon, and when it comes across a class that extends another class it could check against that mapping if a method was overridden and remove the decorator? — Reply to this email directly, view it on GitHub <#3460 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBDB7VF75W7MAYJBRTDVTZS6NANCNFSM53HXDV6A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Gofilord]

Sounds like a potential idea, the two problems I can imagine with that is that a) a class declaration name doesn't guarantee that it is used under the same name in extends clause, b) there might be multiple classes with the same name. c) it only works in the lucky case the superclass file is processed before the subclass file.
…

I ended up doing what I said. I had to run the codemod a couple of times (to fix the process ordering issue), and then I was still left with some errors, but most of the work was done. Also I saved the mapping in a JSON file to survive between different codemod executions (each of them fires up a new instance of the codemod)