Adding decorators support #271

aminya · 2021-04-06T17:36:53Z

This adds decorators support such as for mobx. I have added the test which was taken from a real-world project.

Fixes #143

codecov · 2021-04-06T17:41:12Z

Codecov Report

Merging #271 (23ced32) into master (ea69a99) will increase coverage by 93.64%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #271       +/-   ##
===========================================
+ Coverage        0   93.64%   +93.64%     
===========================================
  Files           0        9        +9     
  Lines           0      614      +614     
  Branches        0       99       +99     
===========================================
+ Hits            0      575      +575     
- Misses          0       39       +39

Impacted Files	Coverage Δ
src/cli.ts	`100.00% <ø> (ø)`
src/convert.ts	`95.23% <ø> (ø)`
src/detect-jsx.ts	`100.00% <ø> (ø)`
src/transform.ts	`88.88% <ø> (ø)`
src/transforms/declare.ts	`97.29% <ø> (ø)`
src/transforms/object-type.ts	`97.93% <ø> (ø)`
src/transforms/react-types.ts	`97.56% <ø> (ø)`
src/transforms/utility-types.ts	`97.56% <ø> (ø)`
src/util.ts	`94.44% <ø> (ø)`
... and 8 more

aminya · 2021-04-06T18:04:02Z

@kevinbarabash Would you please check this out? 😃

aminya · 2021-04-06T18:07:43Z

.prettierignore

@@ -2,6 +2,7 @@ test/fixtures/convert/formatting
 test/fixtures/convert/imports/named_import_specifier_kind/
 test/fixtures/convert/function-types/predicate03
 test/fixtures/convert/imports/dynamic_import
+test/fixtures/convert/decorator/


Just for the record, this was added because prettier removed the @observable from above the markersMapping in the test when the parser is flow.

Fortunately, it works fine with TypeScript, which is what flow-to-ts uses in the end for formatting with prettier.

kevinbarabash

Thanks for the PR. It's cool that transform.ts didn't need to be changed. There are maybe a few more test cases that might be worth adding:

multiple decorators
decorator on a class
decorator factories

package.json

test/fixtures/convert/decorator/mobx/flow.js

dimitropoulos · 2021-04-07T12:49:53Z

Just want to chime in that I would benefit quite a lot from this in the conversion of insomnia: Kong/insomnia#3266

For now, we'll be using a minimal implementation of this PR dimitropoulos@41a52dc (I made it a few days ago before this PR existed).

We'll need to be using this code in our conversion so it'd be useful if it could be merged soon.

I'd be happy to help however possible.

aminya · 2021-04-07T20:07:51Z

Just want to chime in that I would benefit quite a lot from this in the conversion of insomnia: Kong/insomnia#3266

@dimitropoulos Could you give us some simple examples of the decorators you use and the expected output so we can add them to the tests?

aminya · 2021-04-07T20:48:19Z

Thanks for the PR. It's cool that transform.ts didn't need to be changed. There are maybe a few more test cases that might be worth adding:

multiple decorators

decorator on a class

decorator factories

Done!

dimitropoulos · 2021-04-07T20:49:55Z

Thankfully we only have exactly one decorator, autoBindMethodsForReact from https://www.npmjs.com/package/class-autobind-decorator, that we use in 148 places.

To pick a random example: https://github.com/Kong/insomnia/blob/develop/packages/insomnia-app/app/ui/components/editors/password-editor.js#L24

aminya · 2021-04-07T21:00:32Z

Thankfully we only have exactly one decorator, autoBindMethodsForReact from npmjs.com/package/class-autobind-decorator, that we use in 148 places.

OK. I added the test for that one too.

kevinbarabash · 2021-04-08T00:27:08Z

test/fixtures/convert/decorator/mobx/flow.js

+import { observable, computed, action } from "mobx";
+export class Store {
+  @observable
+  markersMapping: Map<number, MarkerStore> = new Map();
+
+  @computed
+  get filePath(): ?string {
+    return "/file";
+  }
+
+  @action
+  newMarkerStore(editorId: number) {
+    return 1;
+  }
+}


Thanks for simplifying these examples. The could be simpler since the code doesn't have to be semantically valid (in this case the getter doesn't return a value, e.g.

class MyClass { @property num: number = 5; @computed get foo(): string {} @action doSomething() {} }

What you have is fine for now. No action req'd.

kevinbarabash · 2021-04-08T00:27:22Z

test/fixtures/convert/decorator/decorator-on-class-with-argument/flow.js

+@autoBindMethodsForReact(AUTOBIND_CFG)
+class PasswordEditor extends React.PureComponent<Props, State> {


It seems like the decorator-factory example already covers this, unless I'm missing something.

I can remove it if you prefer fewer tests, but usually, more tests are better 😄

It's fine. It'd be a different story if the tests took a long time to run, but that isn't the case.

kevinbarabash

LGTM. Thanks for adding this feature. 🙂

kevinbarabash · 2021-04-08T02:30:35Z

I've published 0.5.1 to npm.

dimitropoulos · 2021-04-08T16:10:17Z

wow this is great news, now I don't have to publish a fork just for our use-case! thank you all so much!

aminya added 3 commits April 6, 2021 12:43

Add decorators support

84c0e98

Add missing babel dependency

d8c5f0a

Add submodule update to prepare

4cbd1f5

aminya force-pushed the decorators branch from 2f028ef to 4cbd1f5 Compare April 6, 2021 17:44

Add tests for decorators

e99b8bf

aminya marked this pull request as ready for review April 6, 2021 17:54

aminya commented Apr 6, 2021

View reviewed changes

kevinbarabash reviewed Apr 7, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

test/fixtures/convert/decorator/mobx/flow.js Outdated Show resolved Hide resolved

Simplify the test

1b15540

aminya force-pushed the decorators branch from 8b871db to 1b15540 Compare April 7, 2021 20:12

aminya added 3 commits April 7, 2021 15:29

Add multi-decorator test

93c516e

Add decoratory factory test

1e90f17

Add decorator on a class test

da17a13

Add decorator on a class with argument test

ade78a3

kevinbarabash reviewed Apr 8, 2021

View reviewed changes

Remove charcodes and add babel install to prepare

23ced32

kevinbarabash approved these changes Apr 8, 2021

View reviewed changes

kevinbarabash merged commit b2949cf into Khan:master Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding decorators support #271

Adding decorators support #271

aminya commented Apr 6, 2021 •

edited

Loading

codecov bot commented Apr 6, 2021 •

edited

Loading

aminya commented Apr 6, 2021

aminya Apr 6, 2021 •

edited

Loading

kevinbarabash left a comment

dimitropoulos commented Apr 7, 2021

aminya commented Apr 7, 2021

aminya commented Apr 7, 2021

dimitropoulos commented Apr 7, 2021

aminya commented Apr 7, 2021

kevinbarabash Apr 8, 2021 •

edited

Loading

kevinbarabash Apr 8, 2021

aminya Apr 8, 2021

kevinbarabash Apr 8, 2021

kevinbarabash left a comment

kevinbarabash commented Apr 8, 2021

dimitropoulos commented Apr 8, 2021

		@autoBindMethodsForReact(AUTOBIND_CFG)
		class PasswordEditor extends React.PureComponent<Props, State> {

Adding decorators support #271

Adding decorators support #271

Conversation

aminya commented Apr 6, 2021 • edited Loading

codecov bot commented Apr 6, 2021 • edited Loading

Codecov Report

aminya commented Apr 6, 2021

aminya Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

kevinbarabash left a comment

Choose a reason for hiding this comment

dimitropoulos commented Apr 7, 2021

aminya commented Apr 7, 2021

aminya commented Apr 7, 2021

dimitropoulos commented Apr 7, 2021

aminya commented Apr 7, 2021

kevinbarabash Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

kevinbarabash Apr 8, 2021

Choose a reason for hiding this comment

aminya Apr 8, 2021

Choose a reason for hiding this comment

kevinbarabash Apr 8, 2021

Choose a reason for hiding this comment

kevinbarabash left a comment

Choose a reason for hiding this comment

kevinbarabash commented Apr 8, 2021

dimitropoulos commented Apr 8, 2021

aminya commented Apr 6, 2021 •

edited

Loading

codecov bot commented Apr 6, 2021 •

edited

Loading

aminya Apr 6, 2021 •

edited

Loading

kevinbarabash Apr 8, 2021 •

edited

Loading