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

[DOC beta] Registry and Container Reform #12336

Closed
4 tasks done
mixonic opened this issue Sep 13, 2015 · 22 comments
Closed
4 tasks done

[DOC beta] Registry and Container Reform #12336

mixonic opened this issue Sep 13, 2015 · 22 comments

Comments

@mixonic
Copy link
Member

mixonic commented Sep 13, 2015

2.1 should land in about two weeks. The following docs are lacking:

  • Initializer docs/guides must be updated to accept only application as an argument
  • Instance Initializer docs/guides must be updated to accept applicationInstance as the argument
  • Guides must be audited emberjs/guides#789
  • Requires deprecation guide entries for all relevant items

The main guidance existing today is the blog post for 2.1 beta, which also links to RFCs and PRs.

Please tag PRs for this with [DOC beta]

@mixonic mixonic changed the title [DOCS] Registry and Container Reform [DOC beta] Registry and Container Reform Sep 13, 2015
@mixonic
Copy link
Member Author

mixonic commented Sep 13, 2015

/cc @dgeb, I've also sent the bat signal out to the docs team.

@rwjblue
Copy link
Member

rwjblue commented Sep 13, 2015

  • Initializer docs/guides must be updated to accept only application as an argument
  • Instance Initializer docs/guides must be updated to accept applicationInstance as the argument

Seems to be correct where Ember.Application.initializer and Ember.Application.instanceInitializer are documented:

@rwjblue
Copy link
Member

rwjblue commented Sep 13, 2015

  • Registry methods must be made public and the methods doc'd, or the methods on application should be make public (unsure)

The container and registry are absolutely not being made public.

We have made a small subset of functionality public in a mixin (see here and here).

Those proxy mixins seem to be appropriately documented and marked as @public, but please double check/confirm when you have a chance as we absolutely may have missed something.

@rwjblue
Copy link
Member

rwjblue commented Sep 13, 2015

  • Deprecated method in registry_proxy should be flagged deprecated

The property that is deprecated is calling any methods on application.registry, but that was never documented (either public or private). Where would we document that? Are you suggesting to add documentation for Ember.Application#registry just to mention that it is deprecated?

  • Requires deprecation guide entries for all relevant items

Agreed.

@rwjblue
Copy link
Member

rwjblue commented Sep 13, 2015

I submitted #12337 with screenshots to ensure Ember.Application and Ember.ApplicationInstance have the proper linkages and docs.

@mixonic
Copy link
Member Author

mixonic commented Sep 13, 2015

Updated after confirming some of Robert's feedback.

The property that is deprecated is calling any methods on application.registry, but that was never documented (either public or private). Where would we document that? Are you suggesting to add documentation for Ember.Application#registry just to mention that it is deprecated?

https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/mixins/registry_proxy.js#L14 shows RegistryProxy as public. I was suggesting that RegistryProxy itself should likely not be public.

@rwjblue
Copy link
Member

rwjblue commented Sep 13, 2015

https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/mixins/registry_proxy.js#L14 shows RegistryProxy as public. I was suggesting that RegistryProxy itself should likely not be public.

That was just added in #12337 (because I had to add @class to link properly with Ember.Application and Ember.ApplicationInstance), I'll update.

@rwjblue
Copy link
Member

rwjblue commented Sep 13, 2015

Changed RegistryProxyMixin and ContainerProxyMixin to be @private at the class level (methods are still @public) in #12339.

@acorncom
Copy link
Contributor

[ ] Guides must be audited

Just ran through the guides. As far as I can see, we aren't using initializers anywhere (nor do we want to per @locks if I remember correctly). @locks, can you confirm?

@mixonic
Copy link
Member Author

mixonic commented Sep 22, 2015

@acorncom they are rather heavily documented here: http://guides.emberjs.com/v2.0.0/services-and-initializers/initializers/ and ideally this is a chance for the docs to improve on that front.

@acorncom
Copy link
Contributor

@mixonic indeed they are, I stand corrected. In my defense, I believe those changes hadn't yet been deployed to the live site when I made the comment :-)

@acorncom
Copy link
Contributor

@mixonic I've just opened a guides issue for the docs team, do you want to reference it in the checklist above? Due to the way guides and versioning currently work, we may have to have a pull request pending and merge it as soon as 2.1 is released ...

@mixonic
Copy link
Member Author

mixonic commented Sep 23, 2015

@acorncom updated, thanks :-)

@michaelrkn
Copy link
Contributor

If this is a 2.1 feature, the guides documentation needs to land before 2.1 is released, so that it is documented. In fact, I believe the current contract is "no new features without documentation", so not having this documented in the Guides should probably be a 2.1 blocker.

@rwjblue
Copy link
Member

rwjblue commented Sep 23, 2015

@michaelrkn - The train stops for no one 😝. We can either remove the feature from 2.1 or get the guides updated (API documentation has been addressed in PR's notated above). We have a week or so to get it done, all we need is a volunteer 👯 ...

@acorncom
Copy link
Contributor

For future changes like this, can we get an issue opened in the guides repo? Not sure any of us knew that this was coming (I just happened to be browsing over here and noticed the docs mention) ...

@dgeb
Copy link
Member

dgeb commented Sep 23, 2015

Sorry to not chime in here until now. I'm willing to help out where needed.

@michaelrkn
Copy link
Contributor

@dgeb can you take on emberjs/guides#789?

@dgeb
Copy link
Member

dgeb commented Sep 23, 2015

@michaelrkn ok, will do 👍

@mixonic
Copy link
Member Author

mixonic commented Oct 3, 2015

A deprecation guide is still needed this weekend.

@rwjblue
Copy link
Member

rwjblue commented Oct 4, 2015

Closing as I believe all the items are addressed. @mixonic please reopen if you see something I have missed....

@rwjblue rwjblue closed this as completed Oct 4, 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

No branches or pull requests

5 participants