Skip to content
This repository was archived by the owner on Mar 22, 2019. It is now read-only.

Mention that Ember prototype extensions are optional early.#841

Merged
rwjblue merged 1 commit intoemberjs:masterfrom
pwnall:prototype-panic
Sep 9, 2013
Merged

Mention that Ember prototype extensions are optional early.#841
rwjblue merged 1 commit intoemberjs:masterfrom
pwnall:prototype-panic

Conversation

@pwnall
Copy link
Contributor

@pwnall pwnall commented Sep 9, 2013

While reading the Ember guides, the code samples in Computed Properties made me uneasy, as it seemed to me that Ember modifies Function's prototype without warning. When I got to Observers, I saw that prototype extensions are optional. That made me happy. I's very nice for adding Ember to a large existing codebase.

This change:

  1. Adds the language around prototype modification found in Observers to Computed Properties, to put other reader's minds at ease as soon as the issue surfaces.
  2. Links to the relevant Ember configuration section, so interested readers can check it out right away.

@rwjblue
Copy link
Member

rwjblue commented Sep 9, 2013

👍

rwjblue added a commit that referenced this pull request Sep 9, 2013
Mention that Ember prototype extensions are optional early.
@rwjblue rwjblue merged commit 375eb9f into emberjs:master Sep 9, 2013
@stefanpenner
Copy link
Member

It might also be worth mentioning, that all of ember internals are written to assume prototype extensions are disabled. Additionally, and can likely be omitted, but out tests run with both prototype extensions on and off.

@trek
Copy link
Member

trek commented Sep 9, 2013

This goes a bit against the trend that @tomdale and I purposefully set of the guides being focused on idiomatic and typical use so they can remain short.

I'm not saying this content shouldn't be included in a guide somewhere. I 1000% understand some people's reservations about extending native prototypes. But, in practice, it's not close enough to everybody to warrant inclusion in the a guide about computed properties given the current guide style and goals.

There are many topics I'd like to see mentioned in the guides, but they're excluded to better serve the goal of succinctness. If we'd like the guides to start covering edge case topics and features, we should make the conscious decision of mutating the guides to something more in-depth like Rails Guides, rather than arrive piecemeal.

One idea we've bandied about was having every guide have a complementary more info than you require guide (or maybe section) that explores each topic in a greater depth.

This probably warrants a discussion.

@rwjblue
Copy link
Member

rwjblue commented Sep 9, 2013

@trek - Understood. Thanks for chiming in (I'll be a bit more cautious on merging non-typos going forward)! Shall I revert?

@twokul
Copy link
Member

twokul commented Sep 9, 2013

@rjackson actually the first draft that I wrote explained Ember.computed but it didn't land in the guide because the guide is for newbies.

To be honest, I would revert. But I would add a section that mentions alternative syntax of defining CP.

@trek @tomdale what do you think guys?

@rwjblue
Copy link
Member

rwjblue commented Sep 9, 2013

OK, just reverted.

@trek
Copy link
Member

trek commented Sep 9, 2013

@pwnall
Copy link
Contributor Author

pwnall commented Sep 9, 2013

I prepared this change because of the reference in Observers, which is just two sections after Computed properties.

I think that having the information in Observers but not in Computed properties still goes against the direction suggested by @trek, and has the disadvantage that people like me will have already spent time wondering about prototype modification.

A solution that I propose (and I can prepare a PR for) is to remove the text from Observers, and instead have a short paragraph in Computed properties stating that prototype extensions are optional. Alternatively, if this level of detail is not desirable in the Guides, the paragraph and example in Observers should be removed. Like I said, I'll be happy to prepare a PR either way, to save future readers some effort.

My perspective: I am an Ember noob (haven't written one app yet, just reading the guides), but I have worked with large JavaScript codebases in the past. This information is probably irrelevant to someone learning Ember as their first attempt at writing Web applications, but will probably come into the minds of other Ember noobs who have experience with JavaScript, and are considering adopting Ember into an existing codebase at work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants