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

Decorator Support RFC #440

Merged
merged 8 commits into from Mar 15, 2019

Conversation

Projects
None yet
7 participants
@pzuraq
Copy link
Contributor

pzuraq commented Feb 7, 2019

rendered

pzuraq added some commits Feb 7, 2019

@MelSumner MelSumner referenced this pull request Feb 7, 2019

Open

Octane Tracking Issue #17234

78 of 128 tasks complete
```

This is the most stable part of the spec, and the one that average developers
will come into contact with most. It means that changes to the spec will likely

This comment has been minimized.

@mike-north

mike-north Feb 8, 2019

Contributor

is the before-export and after-export discussion still underway, or has that settled? This is a part of the invocation syntax that has changed a bit over recent months

This comment has been minimized.

@pzuraq

pzuraq Feb 8, 2019

Author Contributor

Stage 2 settled on decorators after, but this could change since this has been a point of contention. However, no decorators in Ember itself are currently class decorators, and there aren't any plans currently to add them. Decorators in external libraries such ember-decorators will experience some amount of churn, but this should be very easy to codemod so it shouldn't have a huge impact.

This is the most stable part of the spec, and the one that average developers
will come into contact with most. It means that changes to the spec will likely
affect the _application_ of decorators, which will in turn affect library and
framework maintainers, but **not** end users in most cases. This places the

This comment has been minimized.

@mike-north

mike-north Feb 8, 2019

Contributor

This statement relies on the assumption that end users will not have decorators to maintain. Is there anything we can do to lower risk in this area, so that we can have more confidence around insulating application developers from changes in decorator specs?

This comment has been minimized.

@pzuraq

pzuraq Feb 8, 2019

Author Contributor

The most we can do in this area is ensure that we develop solid libraries of decorators that are compose-able and allow users to define their own macros without actually having to write decorator code.

For instance, earlier versions of ember-decorators did not provide a way to write custom decorator macros. If a user want to do the equivalent of something like:

function join(...keys, separator = ',') {
  return computed(...keys, function() {
    return keys.map(k => this.get(k)).join(separator);
  });
}

They would have had to write a macro. With our current design, both in Ember and ember-decorators, the above will Just Work, meaning that users are insulated from having to know the specifics of decorators' implementation.

I believe in most cases we'll find that very few user apps actually need to write their own decorators. Ember has been using the decorator pattern for years, but we haven't felt the need to make the internal decorator (Descriptor) system public. There have only been two external users of this (private API) system, both addons - ember-concurrency and ember-intl. Coupled with the lack of demand, I think this demonstrates that decorators are primarily a library/addon level feature, not an app level feature.

This comment has been minimized.

@pzuraq

pzuraq Feb 8, 2019

Author Contributor

We can definitely add that it is recommended to wait to write your own decorators until after they have stabilized in our documentation, to drive this point home.

Show resolved Hide resolved text/0000-decorator-support.md Outdated
provided by Babel. Currently this is the `nov-2018` version, but by the time
decorators are released this may change.

The decorators version will be configurable on a _per-app/engine/addon_ basis.

This comment has been minimized.

@mike-north

mike-north Feb 8, 2019

Contributor

In the scenario where ember-myaddon requires one version of decorators, and ember-myapp requires another:

  • are we concerned about potential asset size increases, resulting from allowing each engine/addon to define its own decorator version?
  • as an addon author, how do I decide which decorator version I'd like to support?
    • It seems like by upgrading my addon from decorators v16 to decorators v17, I'd be doing apps on v17 a favor by removing helpers needed to support v16, but introducing new costs for apps using v16.

This comment has been minimized.

@pzuraq

pzuraq Feb 8, 2019

Author Contributor

We are concerned about this, yes, and we have been talking about ways to mitigate it. The current strategy we have in mind is to allow users to define or build multiple files for each decorator version implemented, and then have a babel transform rewrite the import paths to the version desired in a given addon. Combined with the ongoing work to revamp the build system for packaging and treeshaking, this would allow us to treeshake out any fully unused versions.

This plan is not outlined here however because it may not be possible, and relies on too many unknowns. In the worst case, we may have to continue shipping multiple versions of the transforms for some amount of time. Given that the transforms do not update often, and we are retaining the ability to drop support for versions after LTS releases, this should not amount to much extra weight of code.


The decorators version will be configurable on a _per-app/engine/addon_ basis.
This will allow the ecosystem to adopt decorators without fears that if an
application updates, it could break their decorators. If no version is

This comment has been minimized.

@mike-north

mike-north Feb 8, 2019

Contributor

This will allow the ecosystem to adopt decorators without fears that if an
application updates, it could break their decorators.

If decorators are included in the framework, I'd expect them to play by the same SemVer rules around breaking changes as everything else. Are we defining this as a non-major-release change? Seems like there may be some qualifications here -- if so can we call them out explicitly?

This comment has been minimized.

@pzuraq

pzuraq Feb 8, 2019

Author Contributor

Decorators included in the framework will play by the same SemVer rules, which is why allowing the default to "float" forward is fine. Decorators that are provided by external libraries may have different constraints, which is why you can explicitly lock to a specific version of the decorators transform.

This comment has been minimized.

@jrjohnson

jrjohnson Feb 15, 2019

Should this say:

This will allow the ecosystem to adopt decorators without fears that if an
addon updates, it could break their decorators

This comment has been minimized.

@pzuraq

pzuraq Feb 15, 2019

Author Contributor

This is referring to applications updating their version of Ember, and the transforms for decorators along with it. Addons locking their version of the transforms is a preventative measure against this.

provided by Babel. Currently this is the `nov-2018` version, but by the time
decorators are released this may change.

The decorators version will be configurable on a _per-app/engine/addon_ basis.

This comment has been minimized.

@mike-north

mike-north Feb 8, 2019

Contributor

The decorators version will be configurable on a per-app/engine/addon basis.

If I choose not to explicitly configure, am I opening myself up to some of the potential breakage described below,

if an application updates, it could break their decorators

or significant, unexpected asset size increases?

This comment has been minimized.

@pzuraq

pzuraq Feb 8, 2019

Author Contributor

If you choose not to configure and are only using Ember's official decorators, you are not opening yourself up to potential breakage. You will always be on the latest version of the transforms, and Ember will always ship that version.

If you use an external decorator libraries, you may be opening yourself up to breakage. It depends on whether or not the library authors have updated their decorators.

Asset sizes may increase, but they will likely not be significant. It's unlikely that any library will need to reimplement all of their decorator code, it's much more likely that they will have to write thin translation layers for each version supported.

for apps/addons to import the correct version. Additionally, Ember will attempt
to provide addon authors with tools that allow _them_ to do this as well, so
they can support multiple versions of transforms easily and transparently. The
exact API for these tools is out of scope for this RFC, and will likely be

This comment has been minimized.

@mike-north

mike-north Feb 8, 2019

Contributor

The exact API for these tools is out of scope for this RFC

Shouldn't a proposed implementation strategy or at least a set of user stories (not specific to any specific decorator spec version) for these tools be included in this RFC?

i.e.,

  • The asset size costs for an addon should not scale with the number of decorator versions it supports

This comment has been minimized.

@pzuraq

pzuraq Feb 8, 2019

Author Contributor

I don't believe we can make that kind of guarantee at this point, which is why this RFC is defining the semantics of what we want to support and not focusing on the specifics of the implementation. It may be that supporting multiple versions of decorators does require larger asset sizes, and it will be the responsibility of each addon/app author to weigh the pros and cons of adding more library code to their payloads.

We could maybe outline user stories with the general goals of what we want to achieve, with a caveat that this will be a best-effort goal and not a hard requirement.

users can lock their version if they are using custom decorators. This should be
included _early_ in the guides.

We should also provide thorough documentation for any tooling provided to help

This comment has been minimized.

@mike-north

mike-north Feb 8, 2019

Contributor

Specific things I can think of that are probably worth teaching:

  • Do addon CI matricies need to expand to D (decorator version) ⨉ F (framework version) now?
  • If an app pins their decorator version to maintain compatibility w/ an addon, how will they know when it's a good idea to un-pin? If we don't automate this in some way, it would be a good idea to teach it
addon authors write their own decorators. This will be a bit more of a moving
target, since this RFC does not define exact APIs and they will not be a
priority until Ember itself prepares to support multiple versions of the
transforms.

This comment has been minimized.

@mike-north

mike-north Feb 8, 2019

Contributor

this RFC does not define exact APIs and they will not be a priority until Ember itself prepares to support multiple versions of the transforms

Will there be an additional RFC for this?

This comment has been minimized.

@pzuraq

pzuraq Feb 8, 2019

Author Contributor

I think it depends on a few things:

  1. The complexity of the APIs
  2. The number of users in the ecosystem who will have to adopt them

If there are relatively few addons who end up writing their own decorators, and the APIs are relatively simple and folks are aligned on the solution, the RFC process may not make sense here, since the APIs would be temporary.

@mike-north

This comment has been minimized.

Copy link
Contributor

mike-north commented Feb 8, 2019

Seems like this RFC aims to optimize for several things

  • Reducing the chance for breakage around decorators
  • Aligning with the most recent decorator spec, even while it's in stage 2
  • Following the decorator spec as it evolves over time
  • Ensuring there's almost zero maintenance cost for end users around decorators as the spec evolves
  • Ensuring addons that make use of decorators can be consumed in all apps that have decorator support, even if decorator versions mismatch

And a couple of others that do not appear to be optimized for (only because they're not called out in the RFC)

  • Minimizing the increase to static asset sizes, resulting from adding decorator support
  • Maintaining compatibility with both JS and TS decorators

Given that it's possible we may not be able to solve for all of these things at once, can we create a prioritization for these kinds of goals?

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Feb 8, 2019

Absolutely, I think a clear outlining of constraints we are optimizing for will definitely help us in the future. I will call out right now that "Maintaining compatibility with both JS and TS decorators" is not a goal - with ember-cli-typescript now using babel transforms, we are only seeking to maintain compatibility with the actual spec.

mike-north and others added some commits Feb 8, 2019

Update text/0000-decorator-support.md
Co-Authored-By: pzuraq <me@pzuraq.com>

@amyrlam amyrlam referenced this pull request Feb 13, 2019

Merged

Ember Times No. 85 - February 15th 2019 #3807

5 of 18 tasks complete

@rwjblue rwjblue added the T-framework label Feb 14, 2019

@jrjohnson

This comment has been minimized.

Copy link

jrjohnson commented Feb 15, 2019

I'm not sure the ability for addons to create / version / update decorators is important at this point. Are there a bunch of addons just waiting to get custom decorators into the wild? Since so much of this RFC is wrapped up in the details of keeping decorator version up to date and in sync I assume a lot of effort will need to be spent on this as well.

Wouldn't it be easier to just support official ember decorators right now?

Since the invocation syntax isn't likely to change this would mean the community could fully embrace the official ember decorators knowing that they will be kept up to date as the spec changes. Then when this reaches stage-3 in TC39 support for addons and apps to define their own decorators could be opened up and documented.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Feb 15, 2019

There are at least two fairly high profile addons that already use decorators in the classic world:

  • ember-concurrency
  • ember-intl

By extending ComputedProperty. Without a way to define their own decorators, these addons (and their users) won't have any path to updating to native classes. There are also a number of non-official Ember decorator addons already popping up, such as ember-lifeline-decorators, and many of the decorators in ember-decorators have no equivalent in Ember. Things like @tagName, @className, @observes, etc.

We could allow decorators to be written and applied, but not provide any support and rely on addon authors keeping their decorators up to date, but that seems like it would hamper the community. We could also allow a specific set of community decorators to be used, but that seems highly arbitrary. That's why we ended up on the proposal in its current form.

@jrjohnson

This comment has been minimized.

Copy link

jrjohnson commented Feb 15, 2019

@pzuraq I think the level of complexity in keeping up to date identified by @mike-north is a much larger hindrance to community adoption than it would be to tell addons that they are on their own for staying up to date with changes to the spec.

Speaking only for myself I'm highly unlikely to update my app to native classes / decorators with these complicated caveats. It is simply too much for me to worry about and learn. This is coming from someone who is very excited to make this change!

I would be much more willing to trust Ember's official decorators along with those from these significant major addons that I believe the community will support any moves. I don't think

We could allow decorators to be written and applied, but not provide any support and rely on addon authors keeping their decorators up to date

is actually a bad path. There is risk in adopting decorators at this point and mitigating that risk across experimental addons adds complexity that seems scary to me.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Feb 15, 2019

With ember-compatibility-helpers it's definitely an option, I believe the core team will be discussing this today so I'll wait for their feedback, but I'm definitely open to that route.

@jessica-jordan jessica-jordan referenced this pull request Feb 18, 2019

Merged

Ember Times No. 86 - February 22nd, 2019 #3821

11 of 20 tasks complete
@chriskrycho
Copy link

chriskrycho left a comment

This is a solid update; thanks for putting it together! From the perspective of one of the ember-cli-typescript maintainers, this is a solid plan.

I will note that we should be very clear about the differences—and I'll try to make sure we hit this in the docs we're putting together for the final ember-cli-typescript v2 release over the next two weeks—between v1 decorators in Babel and in TS.

Does anyone have a handy guide to those differences? If not, I'll see if we can put one together as part of that documentation process.

@tomdale

This comment has been minimized.

Copy link
Member

tomdale commented Mar 8, 2019

Given that we have a bit more clarity on what's happening with decorators, and that the discussion here has stabilized, we are now moving this RFC to Final Comment Period.

@chancancode

This comment has been minimized.

Copy link
Member

chancancode commented Mar 15, 2019

We decided to accept this RFC at the framework core team meeting today. Thanks @pzuraq for working on this and everyone else for the discussion 🎉

@chancancode

This comment has been minimized.

Copy link
Member

chancancode commented Mar 15, 2019

@pzuraq do you mind renaming the file to match the RFC number?

@chancancode chancancode merged commit 6e2adf7 into emberjs:master Mar 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.