Skip to content

[DOC release] updated docs regarding ember debug methods - #13272

Merged
mixonic merged 1 commit into
emberjs:masterfrom
webark:updated-debug-docs
Apr 9, 2016
Merged

[DOC release] updated docs regarding ember debug methods#13272
mixonic merged 1 commit into
emberjs:masterfrom
webark:updated-debug-docs

Conversation

@webark

@webark webark commented Apr 6, 2016

Copy link
Copy Markdown
Contributor

This brings them all in line with #13180

Was brought up during conversation in ember-cli/rfcs#50 (comment)

Comment thread packages/ember-debug/lib/deprecate.js Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is make the assertion a copy/paste mistake from the assert jsdoc?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it should say deprecation. Is that really the app-space behavior for deprecate in prod?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was to eventually match the behavior to be implemented by ember-cli/rfcs#50. Although it's not clear from the discussion thus far if the impl will remove the calls or noop the impl.

We recently came across this when we removed all custom deprecation handlers and the default Ember console.log implementation started to flood our prod consoles. The docs definitely mislead us in this case.

I took from my slack conversation that this RFC was to land quickly. Which might be the motivation not to updated the docs to match current behavior only to change them back after impl?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely not add documentation that is incorrect, but would be if/when some other work lands. Right?

@webark

webark commented Apr 8, 2016

Copy link
Copy Markdown
Contributor Author

Ya, I was simply copying and pasting from what was decided in #13180 and seems like some better wording should be done. The motivation for this PR was two things. First, based off of the current documentation, I thought that Ember.info would behave the same in production, cause it never said elsewise in the documentation. Second, it does seem based off of the wording that it will actually remove the calls, not just turn them into a empty function.

@webark

webark commented Apr 8, 2016

Copy link
Copy Markdown
Contributor Author

I'll update the wording for them though so that it's a little better then a ✂️ 🍯 (didn't have a brush or glue. figure honey could be used as glue in a pinch)

@webark

webark commented Apr 8, 2016

Copy link
Copy Markdown
Contributor Author

was just basing which methods where 'no-op'ed from this. https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/debug.js I left out the debug seal cause there was no documentation on that as is.

@mixonic

mixonic commented Apr 8, 2016

Copy link
Copy Markdown
Member

Seems totally great to move forward with! We should just be sure the docs are accurate 😄

@webark
webark force-pushed the updated-debug-docs branch from cfd2821 to 209c426 Compare April 9, 2016 09:28
@webark

webark commented Apr 9, 2016

Copy link
Copy Markdown
Contributor Author

@mixonic @justinaray Updated to hopefully be more strait forward and less wordy. Once ember-cli/rfcs#50 is complete, the documentation could be updated to reflect the eventual implementation.

Comment thread packages/ember-debug/lib/deprecate.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we note both behaviors?

  • In a production build, this method is defined as an empty function (NOP). Uses of this method in Ember itself are stripped from the ember.prod.js build.

@mixonic

mixonic commented Apr 9, 2016

Copy link
Copy Markdown
Member

I've noted two spots where we should mention the calls being stripped from production, but you should look to the actual list for confirmation:

Thanks! 👏

@webark
webark force-pushed the updated-debug-docs branch from 209c426 to c24c876 Compare April 9, 2016 21:33
@webark

webark commented Apr 9, 2016

Copy link
Copy Markdown
Contributor Author

@mixonic updated! And no worries! 😄

@mixonic
mixonic merged commit 422662d into emberjs:master Apr 9, 2016
@mixonic

mixonic commented Apr 9, 2016

Copy link
Copy Markdown
Member

Thanks @webark!

@webark
webark deleted the updated-debug-docs branch April 10, 2016 00:52
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.

3 participants