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

[QUEST] 🌲Project Trim 🌲 #6166

Closed
65 tasks done
runspired opened this issue Jun 18, 2019 · 14 comments · Fixed by #8487
Closed
65 tasks done

[QUEST] 🌲Project Trim 🌲 #6166

runspired opened this issue Jun 18, 2019 · 14 comments · Fixed by #8487
Assignees
Labels
🏷️ cleanup This PR primarily removes deprecated functionality 🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166

Comments

@runspired
Copy link
Contributor

runspired commented Jun 18, 2019

🌲 Project Trim🌲

As the Ember Community approaches delivery of long awaited features enabling tree-shaking, ember-data needs to take some steps to enable this feature to shed the weight of features that consuming applications do not use.

The first step is to ensure that the packages ember-data provides are properly encapsulated. In many cases, this will both enable support for tree-shaking once available to the broader community and enable manually opting for a subset of packages.

Later steps will ensure that ember-data relies on explicit imports (whether automatically generated at build-time or declared by consuming applications) where currently we rely on resolver lookups.


🌲 Use ember-data without @ember-data/serializer

The serializer package provides reference serializer implementations and a base serializer class that are not required for applications that choose to write a custom serializer implementing the minimal serializer interface. This includes the EmbeddedRecordsMixin and the base Transform class.

Transforms, while specified via attr available from @ember-data/model are a concept specific to these reference serializers and not used anywhere else within ember-data. Custom serializers thus may or may not choose to use them via the same mechanism (owner.lookup('transform:<name>')). Lastly the Transform base class merely shows the interface required for transforms but is not required to be used as a base class.

Action Items


🌲 Use ember-data without @ember-data/adapter

The adapter package provides reference adapter implementations and a base adapter class that are not required for applications that choose to write a custom adapter implementing the minimal adapter interface.

Action Items

  • add a public Typescript interface for the minimal required adapter interface
  • eliminate injection of -rest and -json-api adapter (use re-export pattern for -json-api adapter)
  • 🔒RFC - deprecate default serializers and adapters rfcs#522 deprecate Store.defaultAdapter (-json-api) and the -json-api adapter fallback behavior
  • 🔒 [CHORE tests] simplify setupStore registrations #6353 eliminate -rest adapter which is only available for tests at this point unless consumers manually call adapterFor('-rest').
  • cleanup minimum required adapter interface
    • findMany is optional
    • findBelongsTo is optional
    • findHasMany is optional
    • shouldReloadRecord is optional
    • shouldBackgroundReloadRecord is optional
    • shouldReloadAll is optional
    • shouldBackgroundReloadAll is optional
  • implement adapter deprecations from RFC - deprecate default serializers and adapters rfcs#522
  • test for isInvalidError flag or specific message instead of instanceof InvalidError in store
  • ensure internal usage of errorsArrayToHash uses the private store version
  • (Side Quest) Add tests for the MinimumAdapterInterface
  • move coalescing logic into the adapters from the store.

🌲 Use ember-data without @ember-data/model

While at the time of writing this quest @ember-data/model is the only model class available for use with ember-data based on public APIs, the introduction of custom Model Classes will mean that users who install @ember-data/model alternatives via addon or by writing their own should be able to consume ember-data without using a package which provides a model implementation they do not use.

Enabling use without @ember-data/model requires much more groundwork than that needed for the adapter and serializer package.

Model Action Items

  • move the implementation of Model from the store package into the Model package.
  • move the implementation of @hasMany and @belongsTo into the Model package.
  • move -private/system/relationships/ext.js in @ember-data/store into the Model package
  • move the implementation of ManyArray and PromiseManyArray into the Model package. See RFC notes
  • move the implementation of PromiseBelongsTo into the Model package.
  • inject the Model specific _modelForMixin implementation into the store from the Model package (this provides the multi-inheritance polymorphism feature which some aspects of ember-data currently support).
  • deprecate BelongsToReference.push receiving a Model instance
  • polish off the implementation of the Identifiers RFC.
    • refactor conversion of Record to RecordIdentifier to occur within @belongsTo and @hasMany BLOCKED requires RecordData V2

InternalModel Action Items

  • Deprecate/shim intimate Reference.internalModel property
  • Refactor Store.getReference to not lookup the associated InternalModel to generate the reference.
  • Move the state-machine into the Model package.
  • Refactor store APIs to use identifiers instead of InternalModel
  • Implement RecordData Errors RFC
  • Implement Request State RFC.
  • Move InternalModel into the Model package. (this is the LegacySupport class)

InternalModel Cleanup Action Items

Once the items in InternalModel Action Items are complete, we can move the instantiation and lookup logic in the Store into the Model package.

  • create InternalModel only for @ember-data/model when using the instantiateRecord hook.
  • Eliminate any remaining Store._internalModelForId usage in favor of Identifier based logic.

🌲 Use ember-data without @ember-data/relationship-layer

The relationship-layer package is currently deeply entangled with @ember-data/model's Model implementation. Even though we can encapsulate the imports to/from these packages, users would not be able to use @ember-data/model without also using @ember-data/relationship-layer. Additionally, in its current form the relationship-layer is not abstracted away from the specifics of the implementation of Model making is largely unusable with other Model implementations. Changes to address this are needed but are outside the scope of this quest, and until such time as they occur users wishing to drop this package would need to drop both this and the Model package together.

  • move everything within -private/system/relationships/state in @ember-data/store into a new relationship-layer package. See RFC notes
  • Move belongsToReference and HasManyReference into relationship-layer from store.
  • refactor usage of internalModel to pull information from the RecordIdentifier or the Store for type id hasRecord and getRecord()
  • isolate relationship fetching logic (currently spread across dozens of private store methods) into a private middleware to be used by the private -request-manager service (moved into the model layer to make it easier to write alternative relationship fetching logic for a new relationship layer)

🌲 Use ember-data without @ember-data/debug

Provides the DataAdapter for the ember-inspector

  • refactor to use service injection, kill initializer injection
  • refactor to manage own registration
  • detect should not require Model
  • stop automatically shipping to production (ship by config only)
  • Add documentation for shipping the inspector in production if desired
  • Add Encapsulation Test

🌲 Use ember-data without @ember-data/record-data

Provides the default RecordData implementation

  • move RecordData into this package
  • (stretch goal) Land RecordDataV2 Note: blocker on a key bit of model refactoring, no longer stretch goal
  • (stretch goal) Public Docs for RecordData
  • hook into store package to implement createRecordDataFor
  • Add Encapsulation Test
    • [-] (stretch goal) Move RecordData interface tests into the encapsulation test app

Misc


🌲 Tree Shaking Strategy

embroider is pursuing a strategy in which consuming applications opt in to enabling tree-shaking of specific kinds of features once they have determined for their own application that this is allowable. For certain kinds of things embroider provides an escape hatch to not-shake. Currently this includes models, adapters, and serializers and their usage is not easy to statically analyze. Here we outline our plan for reducing the costs of these primitives, but these are not a requirement to complete ProjectTrim.

Models - Users will be able to opt-in to a new lighter-weight model experience

This model experience would be heavily informed by ember-m3 (which configures ember-data via public APIs to use its own model). Goals would be for model files to be optional, and to make their usage tree-shakeable.

Adapters/Serializers - Once the deprecation of automatic adapter fallbacks is resolved, we could find a way to let embroider know that the adapters in @ember-data/adapter are shakeable if no user adapter imports them. However, we primarily recommend dropping the adapter package entirely in favor of authoring an adapter specific to your application's need. Further design work here is needed but going forward the plan is to introduce a network-layer that allows registration ofuser-supplied network middlewares. Such middlewares, including those provided by addons, would be imported and registered with the request-manager service directly. Since these are static imports being registered to a service, in the long term no custom infrastructure would be needed to support tree-shaking. A legacy middleware would delegate request management to existing adapters/serializers, removal of which would allow for full tree-shaking.

Also in the long-term, serializers would be deprecated in favor of either their corresponding "adapter" replacement handling these concerns or in favor of middlewares specific to normalization and serialization concerns. Similarly, in the long term no custom infra for ensuring static imports for tree-shaking needs would be required.

Transforms - There are three ways for a user to utilize a transform today: attr(<transformName>), owner.lookup('transform:<transformName>') and direct import of a transform followed by Transform.create(). Of these, the third already plays nicely with tree-shaking. For attr and lookup usage we will need to enforce "static string usage" for lookup and and attr(), at which point we can use this to generate an import file of only those transforms that are actually in-use. Alternatively, we may decide to deprecate transforms altogether in favor of transformations which do not require a container lookup.

Also note, if none of the default serializers is in use and there is no project usage of lookup('transform:<transformName>') then it is likely we can drop all transforms even when referenced by attr.

Note that using the adapter/serializer/record-data/model/debug packages is entirely optional already. If you are not using code from these packages and/or would prefer your own implementation of the public-interface they implement then installing @ember-data/store and whatever other @ember-data/ packages you require directly instead of installing ember-data will result in both the code those packages bring not being present and any code the store contains for configuring them to be removed at build-time.


🌲 Tree Shake all of ember-data

  • [-] deprecate defaultStore behavior in Ember no longer required due to injections having been deprecated and removed, but is a nice-to-have.
  • deprecate injected store on controller/route
@lifeart
Copy link

lifeart commented Jul 10, 2019

@runspired
Copy link
Contributor Author

@lifeart hate to tell you this but we've got the codemod and lint rules mostly ready to go already.

rfc-data: https://github.com/ember-data/ember-data-rfc395-data
codemod: https://github.com/dcyriller/ember-data-codemod

I think we still need to update the lint plugin to pull from this rfc-data

@lifeart
Copy link

lifeart commented Jul 11, 2019

@runspired good news! @dcyriller should we move codemod into https://github.com/ember-codemods ?
//cc @rwjblue
// // #topic-codemods

Copy link
Member

rwjblue commented Jul 11, 2019

I created an issue to discuss that over in the codemod repo...

@dcyriller
Copy link
Contributor

dcyriller commented Jul 11, 2019

@lifeart good idea, sure I'll do that. Let me fix a bug and initiate the migration.

@runspired
Copy link
Contributor Author

There is node a lint rule PR here: ember-cli/eslint-plugin-ember#450

@runspired runspired pinned this issue Aug 1, 2019
HeroicEric added a commit to HeroicEric/data that referenced this issue Aug 19, 2019
From emberjs#6166

This refactors some tests so that they don't rely on the `createStore`
test helper.

`createStore` was helpful in the past but is less so now that we have
[nice APIs for dependency injection](https://guides.emberjs.com/release/applications/dependency-injection/).

---

One test was removed from `unit/store/adapter-interop - Store working
with a Adapter` because I don't think it was correct.

Here was the test:

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/unit/store/adapter-interop-test.js#L28-L32

This test was, at one time, testing that it was possible to pass a
factory as the `adapter` property to `Store`. This feature was removed
in emberjs#3191 but the test continued
passing because of the way the `createStore` works.

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/helpers/store.js#L57-L60
HeroicEric added a commit to HeroicEric/data that referenced this issue Aug 19, 2019
From emberjs#6166

This refactors some tests so that they don't rely on the `createStore`
test helper.

`createStore` was helpful in the past but is less so now that we have
[nice APIs for dependency injection](https://guides.emberjs.com/release/applications/dependency-injection/).

---

One test was removed from `unit/store/adapter-interop - Store working
with a Adapter` because I don't think it was correct.

Here was the test:

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/unit/store/adapter-interop-test.js#L28-L32

This test was, at one time, testing that it was possible to pass a
factory as the `adapter` property to `Store`. This feature was removed
in emberjs#3191 but the test continued
passing because of the way the `createStore` works.

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/helpers/store.js#L57-L60
HeroicEric added a commit to HeroicEric/data that referenced this issue Aug 19, 2019
From emberjs#6166

This refactors some tests so that they don't rely on the `createStore`
test helper.

`createStore` was helpful in the past but is less so now that we have
[nice APIs for dependency injection](https://guides.emberjs.com/release/applications/dependency-injection/).

---

One test was removed from `unit/store/adapter-interop - Store working
with a Adapter` because I don't think it was correct.

Here was the test:

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/unit/store/adapter-interop-test.js#L28-L32

This test was, at one time, testing that it was possible to pass a
factory as the `adapter` property to `Store`. This feature was removed
in emberjs#3191 but the test continued
passing because of the way the `createStore` works.

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/helpers/store.js#L57-L60
HeroicEric added a commit to HeroicEric/data that referenced this issue Aug 20, 2019
From emberjs#6166

This refactors some tests so that they don't rely on the `createStore`
test helper.

`createStore` was helpful in the past but is less so now that we have
[nice APIs for dependency injection](https://guides.emberjs.com/release/applications/dependency-injection/).

---

One test was removed from `unit/store/adapter-interop - Store working
with a Adapter` because I don't think it was correct.

Here was the test:

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/unit/store/adapter-interop-test.js#L28-L32

This test was, at one time, testing that it was possible to pass a
factory as the `adapter` property to `Store`. This feature was removed
in emberjs#3191 but the test continued
passing because of the way the `createStore` works.

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/helpers/store.js#L57-L60
HeroicEric added a commit to HeroicEric/data that referenced this issue Aug 22, 2019
From emberjs#6166

This refactors some tests so that they don't rely on the `createStore`
test helper.

`createStore` was helpful in the past but is less so now that we have
[nice APIs for dependency injection](https://guides.emberjs.com/release/applications/dependency-injection/).

---

One test was removed from `unit/store/adapter-interop - Store working
with a Adapter` because I don't think it was correct.

Here was the test:

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/unit/store/adapter-interop-test.js#L28-L32

This test was, at one time, testing that it was possible to pass a
factory as the `adapter` property to `Store`. This feature was removed
in emberjs#3191 but the test continued
passing because of the way the `createStore` works.

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/helpers/store.js#L57-L60
runspired pushed a commit that referenced this issue Aug 22, 2019
From #6166

This refactors some tests so that they don't rely on the `createStore`
test helper.

`createStore` was helpful in the past but is less so now that we have
[nice APIs for dependency injection](https://guides.emberjs.com/release/applications/dependency-injection/).

---

One test was removed from `unit/store/adapter-interop - Store working
with a Adapter` because I don't think it was correct.

Here was the test:

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/unit/store/adapter-interop-test.js#L28-L32

This test was, at one time, testing that it was possible to pass a
factory as the `adapter` property to `Store`. This feature was removed
in #3191 but the test continued
passing because of the way the `createStore` works.

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/helpers/store.js#L57-L60
HeroicEric added a commit to HeroicEric/data that referenced this issue Aug 22, 2019
Part of emberjs#6166
Follow up to emberjs#6347

This replaces usage of the `setupStore` helper for
`tests/integration/relationships/*`.
HeroicEric added a commit to HeroicEric/data that referenced this issue Aug 22, 2019
Part of emberjs#6166
Follow up to emberjs#6347

This replaces usage of the `setupStore` helper for
`tests/integration/relationships/*`.
HeroicEric added a commit to HeroicEric/data that referenced this issue Aug 22, 2019
Part of emberjs#6166
Follow up to emberjs#6347

This replaces usage of the `setupStore` helper for
`tests/integration/relationships/*`.
runspired pushed a commit that referenced this issue Aug 22, 2019
…6361)

Part of #6166
Follow up to #6347

This replaces usage of the `setupStore` helper for
`tests/integration/relationships/*`.
HeroicEric added a commit to HeroicEric/data that referenced this issue Aug 22, 2019
Part of emberjs#6166

This replaces usage of the `setupStore` helper for
`tests/integration/adapter/*`.
@Gaurav0
Copy link
Contributor

Gaurav0 commented May 28, 2020

Is this up to date? It looks like there has been little progress since December 2019.

@ryanolsonx
Copy link

Is this up to date?

@runspired
Copy link
Contributor Author

@ryanolsonx yes it is :)

@runspired
Copy link
Contributor Author

#8078 once merged will complete all internalModel tasks, so I've checked them off. This leaves primarily the RecordData v2 tasks (and request coalescing).

@runspired
Copy link
Contributor Author

#8084 picked up where #8078 left off and finally slays the beast. Polymorphic Relationships RFC (kill mixins!) and RecordData V2 are the next priorities.

@runspired
Copy link
Contributor Author

Once #8122 lands we will have only one last lonely task to complete this epic! It's a tricky one, but its also a key bit of code re-shuffling as we prepare to introduce the replacement for adapter/serializer.

With Trim complete, the focus will primarily shift to the items in the roadmap for 5.0 #8086. However, there's been a long running epic (without a publicly attached issue) to "unplug" from Ember itself, where to "unplug" means that we are no longer dependent on any framework code and could be used by the broader JS community as desired. The 5.0 Roadmap accomplishes the major steps needed for this, but there are tons of things not on that roadmap we could use help with. If folks are interested I can whip up a tracking ticket.

@sandstrom
Copy link
Contributor

sandstrom commented Aug 14, 2022

Awesome work runspired! We've doubled the sponsorship to "Extra Appreciative Of The Work" now. Looking forward to 5.0!

If folks are interested (see runspired's question above), we are willing to sponsor contributions on Ember Data (we're already sponsoring 3 people contributing to Ember Data).

@runspired
Copy link
Contributor Author

At long last 👋

@runspired runspired unpinned this issue Mar 26, 2023
@runspired runspired added 🏷️ cleanup This PR primarily removes deprecated functionality and removed Cleanup labels Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ cleanup This PR primarily removes deprecated functionality 🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants