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

[BUGFIX beta] Warn rather than raise readOnly CPs #10966

Closed
wants to merge 1 commit into from

Conversation

tomdale
Copy link
Member

@tomdale tomdale commented Apr 27, 2015

Previously, CPs with setters would raise an exception if they were
marked as readOnly. This change broke Semantic Versioning because
it caused working app code to break.

For example, if you had a computed property that was read-only, but you
happened to include a second value argument out of habit or
convention, this change would cause your previously-functioning app to
break:

computed(function(key, value) {
  // value is unused
}).readOnly()

The above code example will cause an exception to be raised.

In Ember 2.0 we can change this deprecation back to an assertion.

Previously, CPs with setters would raise an exception if they were
marked as readOnly. This change broke Semantic Versioning because
it caused working app code to break.

For example, if you had a computed property that was read-only, but you
happened to include a second `value` argument out of habit or
convention, this change would cause your previously-functioning app to
break:

```
computed(function(key, value) {
  // value is unused
}).readOnly()
```

The above code example will cause an exception to be raised.

In Ember 2.0 we can change this deprecation back to an assertion.
@rwjblue
Copy link
Member

rwjblue commented Apr 27, 2015

The build failed due to bizarre Travis cache issue. I cleared the cache, and restarted the build.

@rwjblue
Copy link
Member

rwjblue commented Apr 27, 2015

👍 on changing this to a deprecation. We will also likely need to add a blurb about this deprecation to the deprecation guide.

@cibernox
Copy link
Contributor

Before merging this, check #10968. I've changed the assertion check to not affect people using the old CP syntax, only people using the new one.

I do think that calling readOnly over a CP with a setter is land for an assertion rather than a deprecation, the problem is that the change was backwards incompatible. Now it should be safe.

@rwjblue
Copy link
Member

rwjblue commented Apr 27, 2015

@cibernox - I agree that #10968 seems preferable.

@tomdale - Can you review and confirm?

@stefanpenner
Copy link
Member

Ya I prefer @cibernox update

@rwjblue
Copy link
Member

rwjblue commented Apr 28, 2015

Fixed with #10968.

@rwjblue rwjblue closed this Apr 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants