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.

Sign up for GitHub

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

Jump to bottom

Add RFC for deprecation Ember Data initializers #181

Merged
merged 3 commits into from
Jan 25, 2017
Merged

Add RFC for deprecation Ember Data initializers #181

merged 3 commits into from
Jan 25, 2017

Conversation

bmac
Copy link
Member

@bmac bmac commented Nov 22, 2016 •
edited by locks
Loading

Rendered.

Known usage (checked for PR sent)

  • bc-ember-data-sails PR
    • ./bc-ember-data-sails/addon/initializers/ember-data-sails.js: before: 'store',
    • Last updated one year ago.
  • ember-data-complex PR
    • ./ember-data-complex/addon/initializers/data-complex.coffee: before: 'store'
    • Last updated two years ago
  • ember-data-model-fragments PR
    • ./ember-data-model-fragments/app/initializers/model-fragments.js: before: "store",
  • ember-data-sails PR
    • ./ember-data-sails/addon/initializers/ember-data-sails.js: before: 'store',
  • ember-fryctoria PR
    • ./ember-fryctoria/addon/initializers/extend-ds-model.js: before: 'store',
    • ./ember-fryctoria/addon/initializers/syncer.js: before: 'store',
    • ./ember-parse/addon/initializers/parse.js: before: 'store',
  • ember-solr PR
    • ./ember-solr/addon/initializers/solr.js: before: 'store',
    • Last updated two years ago
  • ember-youtube-data-model PR
    • ./ember-youtube-data-model/addon/initializers/youtube.coffee: before: 'store'
    • Last updated two years ago

Todo

  • RFC - PR
    • Submitted
    • FCP
    • Merged (pending FCP)
  • Deprecation Guide - PR
    • Submitted
    • Merged
    • Updated (pending RFC)
  • Code change - PR
    • Submitted
    • Merged (pending RFC)

@bmac bmac force-pushed the deprecate-ember-data-initializers branch from 230e0e4 to 919279f Compare November 22, 2016 21:47
@bmac bmac mentioned this pull request Nov 22, 2016
Merged
1 task
locks
locks requested changes Nov 30, 2016
View reviewed changes
text/0181-deprecate-ember-data-initializers.md
`transforms`, and `store` Ember initializers that Ember Data injects
into apps. The `ember-data` initializer not be changed and any code
that previously depended on the ordering of these initializers (via
the `before` or `after` properties on an initalizer) will can be
Copy link
Contributor

Choose a reason for hiding this comment

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

"will can be changed"

text/0181-deprecate-ember-data-initializers.md
# Summary

The goal of this RFC is to remove the `data-adapter`, `injectStore`,
`transforms`, and `store` Ember initializers that Ember Data injects
Copy link
Contributor

Choose a reason for hiding this comment

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

possibly better to refer to them as application/application instance initializers?

text/0181-deprecate-ember-data-initializers.md

# Motivation

The intializes `data-adapter`, `injectStore`, `transforms`, and
Copy link
Contributor

Choose a reason for hiding this comment

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

initializers

text/0181-deprecate-ember-data-initializers.md
`store` have not been used by Ember Data since
[Apr 8, 2014](https://github.com/emberjs/data/commit/d25e23f622a3677b8372db535b2ab824ad306a16). However,
they are still injected into every Ember app that depends on Ember
Data because existing apps may have depending on these initializers
Copy link
Contributor

Choose a reason for hiding this comment

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

"may have depending"

text/0181-deprecate-ember-data-initializers.md
Data needs to support.

Since these initializers are noop functions that run after the
`ember-data` initializer. Any initializers that depends on one of the
Copy link
Contributor

Choose a reason for hiding this comment

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

comma instead of period?

text/0181-deprecate-ember-data-initializers.md
# Detailed design

Ember Data's instance initializer will start checking for any
initializers who's `before` or `after` properties depend on one of
Copy link
Contributor

Choose a reason for hiding this comment

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

whose

text/0181-deprecate-ember-data-initializers.md
Ember Data's instance initializer will start checking for any
initializers who's `before` or `after` properties depend on one of
these deprecated initalizer. If it finds an initalizer that references
one of the deprecated initalizers Ember Data will then log a
Copy link
Contributor

Choose a reason for hiding this comment

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

initializers,

text/0181-deprecate-ember-data-initializers.md
# How We Teach This

This change should have no impact on how we teach Ember or Ember
Data. The initalizers that will be remove have been unused for a long
Copy link
Contributor

Choose a reason for hiding this comment

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

"will be remove"

Copy link

Choose a reason for hiding this comment

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

Muphry's law kicking in here ... "removed" :trollface:

text/0181-deprecate-ember-data-initializers.md
removed.


# How We Teach This
Copy link
Contributor

Choose a reason for hiding this comment

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

could we come up with a codemod?

text/0181-deprecate-ember-data-initializers.md

Users who need to run initalizer code before or after Ember Data
injects the store into routes should be taught to use `before:
'ember-data'`, or `after: 'ember-data'` on there initalizers.
Copy link
Contributor

Choose a reason for hiding this comment

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

their

Copy link
Contributor

Choose a reason for hiding this comment

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

initializers

@locks
Copy link
Contributor

locks commented Nov 30, 2016

@bmac as done in https://github.com/cibernox/rfcs/blob/deprecate-ember-k/text/0000-deprecate-ember-k.md#addenda, I would like to suggestion reaching out to EmberObserver and get some usage statistics across published addons (worried about outdated dependencies), and coming up with a codemod, if possible. Looking good!

@bmac
Copy link
Member Author

bmac commented Dec 12, 2016

@locks I checked out all of the Ember Data Adapters and Ember Data Extensions form Ember observer and found the following adapters referencing the deprecated initializers.

./bc-ember-data-sails/addon/initializers/ember-data-sails.js:  before: 'store',
./ember-data-complex/addon/initializers/data-complex.coffee:  before: 'store'
./ember-data-model-fragments/app/initializers/model-fragments.js:  before: "store",
./ember-data-sails/addon/initializers/ember-data-sails.js:  before: 'store',
./ember-fryctoria/addon/initializers/extend-ds-model.js:  before:     'store',
./ember-fryctoria/addon/initializers/syncer.js:  before:     'store',
./ember-parse/addon/initializers/parse.js:  before: 'store',
./ember-solr/addon/initializers/solr.js:  before: 'store',
./ember-youtube-data-model/addon/initializers/youtube.coffee:  before: 'store'

@bmac bmac mentioned this pull request Dec 12, 2016
Open
bmac
bmac commented Dec 12, 2016
View reviewed changes
text/0181-deprecate-ember-data-initializers.md

The goal of this RFC is to remove the `data-adapter`, `injectStore`,
`transforms`, and `store` Ember application initializers that Ember Data injects
into apps. The `ember-data` initializer not be changed and any code
Copy link
Member Author

Choose a reason for hiding this comment

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

"will not be changed"

This was referenced Dec 12, 2016
Merged
Merged
Merged
Open
Closed
Open
Merged
@locks locks mentioned this pull request Jan 5, 2017
Closed
@locks locks added the T-ember-data RFCs that impact the ember-data library label Jan 5, 2017
@jenweber jenweber mentioned this pull request Jan 7, 2017
Closed
@locks
Copy link
Contributor

locks commented Jan 9, 2017

The Ember Data team has reviewed this proposal and have decided to move it to the Final Comment Period! If no new significant concerns are raised, in one week's time this proposal will be officially merged.
Thanks everyone for their work!

@igorT igorT merged commit 8f302c7 into emberjs:master Jan 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tragiclifestories tragiclifestories tragiclifestories left review comments

@locks locks locks approved these changes

Assignees
No one assigned
Labels
Final Comment Period T-ember-data RFCs that impact the ember-data library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bmac @locks @tragiclifestories @igorT