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

Mixed programming models considered harmful #364

Closed
4 tasks
mike-north opened this issue Nov 2, 2018 · 6 comments
Closed
4 tasks

Mixed programming models considered harmful #364

mike-north opened this issue Nov 2, 2018 · 6 comments
Labels

Comments

@mike-north
Copy link
Contributor

mike-north commented Nov 2, 2018

RECOMMENDATION: in the 2.0 stable release, ember-cli-typescript should assume/require the presence of ember-decorators in consuming apps

DETAILS: Now that support for ES6 classes is landing in an official way, the picture around best practices is becoming far less fuzzy. Early on, we hedged against use of decorators, due to the ECMA-262 standard still being in flux.

This “hedge” has taken the form of us recommending patterns like

export default class Foo extends Component.extend({
   foo: computed('bar', function() { ... })
}) {

}

const myFoo = new Foo();

The implementors of ES6 support in ember have indicated that this pattern is “discouraged” emberjs/ember.js#17174 (comment) . -- @pzuraq

Several extremely large ember applications are on track to begin adoption ES6 classes with decorators within the next six weeks — with that comes a stronger promise of stability.

In order to ensure we set users up for “stability without stagnation” we should align with this advice, and begin to generate code that encourages patterns like

export default class Foo extends Component {
   @computed('bar')
   get foo() { ... }
}

const myFoo = Foo.create();

The benefits our consumers will get include

  • They’ll benefit from codemods
  • They’ll be set up for forward-compatibility w/ future versions of ember, which may not include EmberObject.extend
  • Potentially (future) massive simplification of types, if we find that we no longer need strong typing around .extend and .reopen. This is public API today, but if we set people up for tomorrow, there will be less upgrade pain for our users

Action Items

  • Figure out how ember-concurrency tasks would work nicely in ES6, as they're the last thing that really needs to be put in an extend block to get type safety and proper functionality.
  • Update blueprints for ES6 only
  • Validate presence of ember-decorators in included hook for ember-cli-decorators v2.0
  • Update README to reflect new advice around not using .extend

Conversation around this w/ maintainers

dfreemanToday at 11:37 AM
👍 that all makes sense
I hadn't thought about how the mix'n'match would complicated the codemod
it leaves ember-concurrency in an awkward place, though

jamescdavisToday at 11:44 AM
I agree on decorators and have leaned that direction for a while.
Yes, we really need to get ember-concurrency decorators stabilized.
Ditto on the thanks for Azure. I know I have a MS account lying around somewhere...

dfreemanToday at 11:45 AM
The issue with the ember-concurrency decorators is that they're less typesafe than putting the tasks in an extend() hash

jamescdavisToday at 11:46 AM
Right. I remember now. It's not just about API.

dfreemanToday at 11:47 AM
right
if decorators learn how to modify the type of the thing they decorate once they get overhauled for stage 3 (or whenever it happens), the story gets better

jamescdavisToday at 11:50 AM
Yep, I was just looking for discussions on that.

dfreemanToday at 11:50 AM
the conventional wisdom is that the TS team isn't going to touch their decorators implementation until the proposal reaches stage 3
but that's been repeated by so many people in so many contexts (myself included) that I no longer remember what they've explicitly said and what was originally speculation on how things would advance when that happens

jamescdavisToday at 11:53 AM
Yeah, I've "heard" that many times as well, but was looking for something concrete about the status. Haven't found anything yet...

dwickernToday at 11:57 AM
I haven't seen any official plan, or any indication that they will change decorators at all
is there a list somewhere of ember codemods which support typescript?

chriskrychoToday at 1:15 PM
I think they've said as much privately to a lot of people; I don't know they've made any public statement to that effect.

@dwickern no public list to my knowledge, though I know Rob Jackson did the work to make it Just Work™ sometime this summer.

I'm confident the TS team will update their implementation once Stage 3 hits, since they need to be spec compliant.

I am not confident they'll make decorators able to affect types. They had some pretty strong pushback back in 2016 or 2017 about it because of the knock-on effects it could have in terms of deep cycles in terms of determining the resulting type of a given class.
@mike N I'm also :thumbup::skin-tone-2: on that inbound issue.

Mike NToday at 1:25 PM
@dfreeman we think the problem can be solved by

class {

@task(function*() { ... })
myTask!: Task<string[]>

}

(edited)

chriskrychoToday at 1:25 PM
Oh! Huh.

dfreemanToday at 1:26 PM
Yeah, I wondered about that

chriskrychoToday at 1:26 PM
That'll be super weird looking, but yes, that should do it.

dfreemanToday at 1:26 PM
Not my favorite, but probably a good approach

Mike NToday at 1:26 PM
we can all agree it looks bad, but ✅ type safety, and ✅ no mixing programming models
👍
1

chriskrychoToday at 1:27 PM
Yeah, I'll 100% take that. Ugly as sin, but I'll take it.
The really annoying bit will be writing better types than mine.
Which work, but not great.

Mike NToday at 1:28 PM
I am told that writing codemods for decorators is trivial, compared to the effort required for handling the mixed programming model case

chriskrychoToday at 1:28 PM
I 100% believe that.
Note: moving this particular discussion public.

@josemarluedke
Copy link
Contributor

I agree with the mixed programing model is bad. We have been using TS and our apps and devs often get mad about the mix of .extend to add ember-concurrency tasks there.

Another thing that I which was better is interacting with mixins. I know mixins should be avoided at this point, but many addons still uses them in the programing model. Therefore, people will still need to use .extend.

@mike-north
Copy link
Contributor Author

If you need mixins, you should be using .extend() and not classes. Mixing the two programming models for a single entity is harmful, but choosing one or the other is fine.

@josemarluedke I encourage you to de-mixinify libraries as you come across them. You can use this as a template for how to engage with them
mainmatter/ember-simple-auth#1619

@pzuraq
Copy link

pzuraq commented Nov 2, 2018

So spoke with Mike about this and I want to clarify real quickly here.

In general, we absolutely want to avoid the extends ... .extend() pattern. It complicates types like crazy, it's very difficult to codemod (as there are many possible permutations of what could be happening in either part of the definition), and subjectively it's just not the prettiest 😛

If you include ember-decorators as part of the programming model, almost every Ember feature can be expressed in a native class definition. The one exception is mixins. This was a conscious decision in the ES Class RFC, because Ember Mixins are integral to the old object model and as such, we will eventually be removing them.

That said, there are lots of usages of mixins, and people will not be able to migrate overnight. So, adding mixins to a class definition is currently the one recommended usage of .extend(). The native class codemod will actually target this as its output.

However, it is more recommended that you migrate off mixins entirely. it'll clean up your types and simplify your code, and in general new code should avoid using mixins (at least until they are part of the language proper).

@Alonski
Copy link
Contributor

Alonski commented Sep 3, 2019

@mike-north @chriskrycho Any updates on this? Especially now that v3 is out :)

@chriskrycho
Copy link
Member

chriskrycho commented Sep 3, 2019

The recommendation remains as it was; we’ll slowly be updating guidance and docs and blueprints to match Octane idioms. In practice, the migrations required to get to Octane all just make TS work that much more nicely and easily, and in general align with this discussion. (Not a surprise, since TS users helped figure out a lot of the constraints and pain points in this space!)

One useful resource will be the Ember Atlas, which is a community resource that @pzuraq and I are building out as we support LinkedIn’s migration to Octane. Where it makes sense, I’m including notes, example code, etc. for TS as “appendix”-style material. Mostly, though, the JS material there itself will get you 90% of where you need to be and the last 10% will be fairly obvious. That’s one of the lovely consequences of Octane moving to native JS idioms throughout!

(If you’re curious about why it’s appendix-style material and not part of the mainline: We want to be clear that TS it not officially supported either internally at LI or officially for Ember. We are slowly clearing the way for it to be officially supported both for us and for the community, though, and having docs in place is part of that. So: appendices!)

@chriskrycho
Copy link
Member

We've now long since updated all docs to assume Octane. Mike's recommendations here were pretty much exactly what we documented and guided people toward, though Ember itself made some of those pitfalls less problematic over time. Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants