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.

Sign up for GitHub

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

[undecorate] partial fix for #3460: @action => override #3478

Merged
merged 9 commits into from
Feb 9, 2023
Merged

[undecorate] partial fix for #3460: @action => override #3478

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0a1a53a
fix
urugator Jul 24, 2022
64baa47
remove test.only
urugator Jul 24, 2022
17a4628
remove commented code
urugator Jul 24, 2022
347d033
Merge remote-tracking branch 'upstream/main' into fix-3460
urugator Dec 21, 2022
334cac3
upgrade jscodeshift, merge main, fix test
urugator Dec 21, 2022
a831316
Merge remote-tracking branch 'upstream/main' into fix-3460
urugator Feb 5, 2023
7137c35
revert changes
urugator Feb 5, 2023
6be6f32
spaces
urugator Feb 5, 2023
15879ff
yarn.lock
urugator Feb 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/many-steaks-eat.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"mobx-undecorate": minor
---

partial fix #3460: replace `action` with `override` if field uses override keyword
32 changes: 16 additions & 16 deletions docs/observable-state.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,22 +217,22 @@ Note that it is possible to pass `{ proxy: false }` as an option to `observable`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably done by precommit pretty hook

## Available annotations

| Annotation | Description |
| ---------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `observable`<br/>`observable.deep` | Defines a trackable field that stores state. If possible, any value assigned to `observable` is automatically converted to (deep) `observable`, [`autoAction`](#autoAction) or `flow` based on it's type. Only `plain object`, `array`, `Map`, `Set`, `function`, `generator function` are convertible. Class instances and others are untouched. |
| `observable.ref` | Like `observable`, but only reassignments will be tracked. The assigned values are completely ignored and will NOT be automatically converted to `observable`/[`autoAction`](#autoAction)/`flow`. For example, use this if you intend to store immutable data in an observable field. |
| `observable.shallow` | Like `observable.ref` but for collections. Any collection assigned will be made observable, but the contents of the collection itself won't become observable. |
| `observable.struct` | Like `observable`, except that any assigned value that is structurally equal to the current value will be ignored. |
| `action` | Mark a method as an action that will modify the state. Check out [actions](actions.md) for more details. Non-writable. |
| `action.bound` | Like action, but will also bind the action to the instance so that `this` will always be set. Non-writable. |
| `computed` | Can be used on a [getter](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get) to declare it as a derived value that can be cached. Check out [computeds](computeds.md) for more details. |
| `computed.struct` | Like `computed`, except that if after recomputing the result is structurally equal to the previous result, no observers will be notified. |
| `true` | Infer the best annotation. Check out [makeAutoObservable](#makeautoobservable) for more details. |
| `false` | Explicitly do not annotate this property. |
| `flow` | Creates a `flow` to manage asynchronous processes. Check out [flow](actions.md#using-flow-instead-of-async--await-) for more details. Note that the inferred return type in TypeScript might be off. Non-writable. |
| `flow.bound` | Like flow, but will also bind the flow to the instance so that `this` will always be set. Non-writable. |
| `override` | [Applicable to inherited `action`, `flow`, `computed`, `action.bound` overridden by subclass](subclassing.md). |
| <span id="autoAction"></span> `autoAction` | Should not be used explicitly, but is used under the hood by `makeAutoObservable` to mark methods that can act as action or derivation, based on their calling context. It will be determined at runtime if the function is a derivation or action. |
| Annotation | Description |
| ------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `observable`<br/>`observable.deep` | Defines a trackable field that stores state. If possible, any value assigned to `observable` is automatically converted to (deep) `observable`, [`autoAction`](#autoAction) or `flow` based on it's type. Only `plain object`, `array`, `Map`, `Set`, `function`, `generator function` are convertible. Class instances and others are untouched. |
| `observable.ref` | Like `observable`, but only reassignments will be tracked. The assigned values are completely ignored and will NOT be automatically converted to `observable`/[`autoAction`](#autoAction)/`flow`. For example, use this if you intend to store immutable data in an observable field. |
| `observable.shallow` | Like `observable.ref` but for collections. Any collection assigned will be made observable, but the contents of the collection itself won't become observable. |
| `observable.struct` | Like `observable`, except that any assigned value that is structurally equal to the current value will be ignored. |
| `action` | Mark a method as an action that will modify the state. Check out [actions](actions.md) for more details. Non-writable. |
| `action.bound` | Like action, but will also bind the action to the instance so that `this` will always be set. Non-writable. |
| `computed` | Can be used on a [getter](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get) to declare it as a derived value that can be cached. Check out [computeds](computeds.md) for more details. |
| `computed.struct` | Like `computed`, except that if after recomputing the result is structurally equal to the previous result, no observers will be notified. |
| `true` | Infer the best annotation. Check out [makeAutoObservable](#makeautoobservable) for more details. |
| `false` | Explicitly do not annotate this property. |
| `flow` | Creates a `flow` to manage asynchronous processes. Check out [flow](actions.md#using-flow-instead-of-async--await-) for more details. Note that the inferred return type in TypeScript might be off. Non-writable. |
| `flow.bound` | Like flow, but will also bind the flow to the instance so that `this` will always be set. Non-writable. |
| `override` | [Applicable to inherited `action`, `flow`, `computed`, `action.bound` overridden by subclass](subclassing.md). |
| <span id="autoAction"></span> `autoAction` | Should not be used explicitly, but is used under the hood by `makeAutoObservable` to mark methods that can act as action or derivation, based on their calling context. It will be determined at runtime if the function is a derivation or action. |

## Limitations

Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,5 @@
"hooks": {
"pre-commit": "pretty-quick --staged"
}
}
}
}
2 changes: 2 additions & 0 deletions packages/eslint-plugin-mobx/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ It's a bit opinionated and can lead to a lot of false positives depending on you
### mobx/no-anonymous-observer (deprecated)

_Deprecated in favor of [react/display-name](https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/display-name.md) + [componentWrapperFunctions](https://github.com/jsx-eslint/eslint-plugin-react). Example of **.eslintrc**:_

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably done by precommit pretty hook

```
{
"rules": {
Expand All @@ -97,6 +98,7 @@ _Deprecated in favor of [react/display-name](https://github.com/jsx-eslint/eslin
}
}
```

---

Forbids anonymous functions or classes as `observer` components.
Expand Down
75 changes: 75 additions & 0 deletions packages/mobx-undecorate/__tests__/undecorate.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,81 @@ describe("action", () => {
}"
`)
})

test("method - override", () => {
expect(
convert(`
import { action } from "mobx"

class Box extends Shape {
constructor(arg) {
super(arg)
}

@action
override method(arg: number): boolean {
console.log('hi')
return true
}
}
`)
).toMatchInlineSnapshot(`
"import { action, override, makeObservable } from \\"mobx\\";

class Box extends Shape {
constructor(arg) {
super(arg)

makeObservable(this, {
method: override
});
}

override method(arg: number): boolean {
console.log('hi')
return true
}
}"
`)
})

test("method - override - keepDecorators", () => {
expect(
convert(
`
import { action } from "mobx"

class Box extends Shape {
constructor(arg) {
super(arg)
}

@action
override method(arg: number): boolean {
console.log('hi')
return true
}
}
`,
{ keepDecorators: true }
)
).toMatchInlineSnapshot(`
"import { action, override, makeObservable } from \\"mobx\\";

class Box extends Shape {
constructor(arg) {
super(arg)
makeObservable(this);
}

@override
override method(arg: number): boolean {
console.log('hi')
return true
}
}"
`)
})
})

describe("observable", () => {
Expand Down
1 change: 1 addition & 0 deletions packages/mobx-undecorate/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"homepage": "https://mobx.js.org/",
"dependencies": {
"@babel/core": "^7.9.0",
"@babel/parser": "^7.18.9",
"@babel/plugin-proposal-class-properties": "^7.8.3",
"@babel/plugin-proposal-decorators": "^7.8.3",
"@babel/plugin-transform-runtime": "^7.9.0",
Expand Down
20 changes: 18 additions & 2 deletions packages/mobx-undecorate/src/undecorate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ export default function transform(
const lines = fileInfo.source.split("\n")
let changed = false
let needsInitializeImport = false
let importOverride = false
const decoratorsUsed = new Set<String>(options?.ignoreImports ? validDecorators : [])
let usesDecorate = options?.ignoreImports ? true : false
let hasReact = options?.ignoreImports ? true : false
Expand Down Expand Up @@ -228,6 +229,9 @@ export default function transform(
const { comments, ...k } = key
const { comments: comments2, ...v } = value
const prop = j.objectProperty(k, v)
if (v.name === "override") {
importOverride = true
}
prop.computed = !!computed
if (isPrivate) {
privates.push(k.name)
Expand Down Expand Up @@ -261,6 +265,9 @@ export default function transform(
if (!mobxImport.specifiers) {
mobxImport.specifiers = []
}
if (importOverride) {
mobxImport.specifiers.push(j.importSpecifier(j.identifier("override")))
}
mobxImport.specifiers.push(j.importSpecifier(j.identifier("makeObservable")))
}
}
Expand Down Expand Up @@ -308,7 +315,7 @@ export default function transform(
const exportDecl = j.exportDefaultDeclaration(newDefaultExportDefExpr.exported)

// re-create the class
let newClassDefExpr = j.classExpression(clazz.id, clazz.body, clazz.superClass)
const newClassDefExpr = j.classExpression(clazz.id, clazz.body, clazz.superClass)
newClassDefExpr.superTypeParameters = clazz.superTypeParameters
newClassDefExpr.typeParameters = clazz.typeParameters
newClassDefExpr.implements = clazz.implements
Expand Down Expand Up @@ -386,7 +393,7 @@ export default function transform(
if (!j.Decorator.check(decorator)) {
return property
}
const expr = decorator.expression
let expr = decorator.expression
if (j.Identifier.check(expr) && !decoratorsUsed.has(expr.name)) {
warn(`Found non-mobx decorator @${expr.name}`, decorator)
return property
Expand All @@ -400,6 +407,15 @@ export default function transform(
property.decorators.splice(0)
}

// Replace decorator with @override
if ((property as any).override) {
const overrideDecorator = j.decorator(j.identifier("override"))
if (options?.keepDecorators) {
property.decorators[0] = overrideDecorator
}
expr = overrideDecorator.expression
}

effects.membersMap.push([
property.key,
expr,
Expand Down