frint: Expose class methods on the app instance level #431

rbardini · 2018-07-12T08:43:33Z

What's done

Add optional methods (Object) property to AppOptions, which has its properties added to the app's this object.
Add app.getMethods method which returns the app's methods object.

Why

Allow access to public methods directly from the app instance, besides the options property.

Closes #426.

fahad19 · 2018-07-12T08:46:56Z

packages/frint/src/App.ts

@@ -183,6 +199,10 @@ export class App {
    return this.getOption('name');
  }

+  public getMethods() {


is this new method necessary?

I think so if for some reason we change the way we save and get those methods, we don't need to change the implementation. Exposing this clear api we avoid future maintainability problems. What do you think?

I am trying to convince myself to keep this, but not yet there :D

at the moment, it is returning the original methods option that is passed to the App's constructor. those functions are not bound to the app instance, and cannot be called directly either. so don't know how they can be useful right away.

I got your point, i thought it was about the way we are exposing this but i agree now that maybe we don't need this method.

Maybe it would be useful for someone who wants to retrieve a list of the public methods exposed by a given app. But yes, not having the methods bound to the app instance makes the return value less useful—and it's accessible via app.getOption('methods') anyway.

I'll remove it for now to avoid confusion. We can write a proposal later on if necessary 🙂

But i don't know, i think we are doing some magic here, seems like a bad smell to me, we now are binding those methods and changing the scope, so we are exposing app instance inside each function, is that the desired behavior?

What do you guys think?

const FooApp = createApp({ name: 'Foo', methods: { // specify what methods this app exposes bar() { this.methods = {}; // exposing app variable console.log('bar') } } })

codecov · 2018-07-12T08:54:08Z

Codecov Report

Merging #431 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
+ Coverage   97.75%   97.76%   +<.01%     
==========================================
  Files         112      112              
  Lines        4366     4380      +14     
==========================================
+ Hits         4268     4282      +14     
  Misses         98       98

Impacted Files	Coverage Δ
packages/frint/src/App.spec.ts	`100% <100%> (ø)`	⬆️
packages/frint/src/App.ts	`99.37% <100%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abb7964...21c4214. Read the comment docs.

fahad19 · 2018-07-12T09:16:28Z

packages/frint/src/App.ts

+      const method = this.options.methods[methodName];
+
+      if (typeof method === 'function') {
+        this[methodName] = method.bind(this);


it will be useful if we check if there is already an existing method with the same name first.

if it exists, we throw an Error.

Good idea, will do 🙂

noisae · 2018-07-12T11:00:51Z

packages/frint/src/App.ts

+          throw new Error(`Cannot overwrite app's \`${methodName}\` property or method with options method.`);
+        }
+
+        this[methodName] = method.bind(this);


I don't know, i think we are doing some magic here, seems like a bad smell to me, we now are binding those methods and changing the scope, so we are exposing app instance inside each function, is that the desired behaviour?

What do you guys think?

const FooApp = createApp({ name: 'Foo', methods: { // specify what methods this app exposes bar() { this.methods = {}; // exposing app variable console.log('bar') } } })

That's an interesting point, and I can see how this may give a lot of freedom to break things. On the other hand, if the framework doesn't automatically bind the methods, the developer could still do and have the same privileges, so I'm not sure if this is a big problem 🤔

But there is another problem here too, if we need to create new methods on this class, and someone is using that name, when we update frintjs, this application is going to break too. We are also breaking the Open Close principle here i think.

regarding open/close:

we are open to others extending the API with further methods.

but if we throw errors whenever there is a conflicting method name, we are closed.

We are really open to modification on this case.
But the thing is, imagine that i create a method called getMethods on my application, and then we do the same on App. Now when i update my frint package my application is going to break.

I think this is not a good thing :/

One possible drawback I foresee is that, once this is merged, any new properties or methods added to the App class would require a major release, since they can potentially clash with injected methods. Should we worry about this, or is the App API stable enough for this to not be an issue?

we haven't added any new methods for months, if not years to the App class.

fahad19 · 2018-07-12T14:56:24Z

Great stuff, @rbardini! 💖

rbardini added feature minor PR labels Jul 12, 2018

rbardini requested review from fahad19 and jcampalo July 12, 2018 08:43

fahad19 reviewed Jul 12, 2018

View reviewed changes

fahad19 assigned rbardini Jul 12, 2018

rbardini force-pushed the expose-class-methods branch from 7c5720b to 89c54e9 Compare July 12, 2018 08:56

fahad19 reviewed Jul 12, 2018

View reviewed changes

noisae reviewed Jul 12, 2018

View reviewed changes

jcampalo approved these changes Jul 12, 2018

View reviewed changes

fahad19 approved these changes Jul 12, 2018

View reviewed changes

rbardini force-pushed the expose-class-methods branch from 56478a8 to 63aaa6f Compare July 13, 2018 08:32

Rafael Bardini added 6 commits July 13, 2018 10:59

Expose class methods on the app instance level

fc9ffb6

Update documentation

1ee7bfb

Fix linting errors

5229a5e

Don't overwrite app fields with options methods

2877cb0

Remove app.getMethods method

0bb6afb

Rename App index signature

21c4214

rbardini force-pushed the expose-class-methods branch from 63aaa6f to 21c4214 Compare July 13, 2018 08:59

rbardini merged commit 0a89cc1 into master Jul 13, 2018

rbardini deleted the expose-class-methods branch July 13, 2018 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frint: Expose class methods on the app instance level #431

frint: Expose class methods on the app instance level #431

rbardini commented Jul 12, 2018

fahad19 Jul 12, 2018

noisae Jul 12, 2018

fahad19 Jul 12, 2018

noisae Jul 12, 2018

rbardini Jul 12, 2018

fahad19 Jul 12, 2018

rbardini Jul 12, 2018

noisae Jul 12, 2018

codecov bot commented Jul 12, 2018 •

edited

Loading

fahad19 Jul 12, 2018

rbardini Jul 12, 2018

noisae Jul 12, 2018 •

edited

Loading

rbardini Jul 12, 2018

noisae Jul 12, 2018 •

edited

Loading

fahad19 Jul 12, 2018

noisae Jul 12, 2018

rbardini Jul 13, 2018

fahad19 Jul 13, 2018

fahad19 commented Jul 12, 2018

frint: Expose class methods on the app instance level #431

frint: Expose class methods on the app instance level #431

Conversation

rbardini commented Jul 12, 2018

What's done

Why

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 12, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisae Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisae Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fahad19 commented Jul 12, 2018

codecov bot commented Jul 12, 2018 •

edited

Loading

noisae Jul 12, 2018 •

edited

Loading

noisae Jul 12, 2018 •

edited

Loading