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

@ember-data packages #395

Open
wants to merge 28 commits into
base: master
from

Conversation

Projects
None yet
@runspired
Contributor

runspired commented Nov 1, 2018

rendered

runspired added some commits Nov 1, 2018

@lupestro

This comment has been minimized.

Contributor

lupestro commented Nov 7, 2018

Are the classes for which there will be no imports ever returned by public methods? If so, is there any scenario where not having an export for the classes returned by public methods prevents doing something we'd reasonably want to do? I can think of an ill-advised testing hack that somebody might use this information to do (shudder) but I'm asking if there's a legitimate case for needing those classes.

@chriskrycho

This comment has been minimized.

chriskrycho commented Nov 7, 2018

Thinking along the lines @lupestro raises, we'll want a story for the TypeScript side of things for types we need to name but which are not importable. I'll have more later, as I'm excited about this.

@runspired

This comment has been minimized.

Contributor

runspired commented Nov 7, 2018

Are the classes for which there will be no imports ever returned by public methods?

@lupestro I think you are asking " are there public classes for which there will be no public imports", to which the answer is YES.

is there any scenario where not having an export for the classes returned by public methods prevents doing something we'd reasonably want to do?

Generally speaking, public classes that don't have public import paths are ones which require extensive setup and lifecycle management by the store and for which it would be infeasible for end users to generate one except via the store.

For the most part, public store interfaces are available for accessing instances of these things, where public methods are not available these gaps should be addressed. The common case folks point out is snapshots, which is something we have in our sights for addressing soon.

@tylerturdenpants tylerturdenpants referenced this pull request Nov 12, 2018

Merged

The Ember Times - Issue No. 73 #3684

6 of 12 tasks complete
@mehulkar

This is great, we're already using some mixture of direct imports and DS, and it was never quite clear to me what the difference was (or why the guides don't show import Model from 'ember-data/model, for example)

Show resolved Hide resolved text/0000-ember-data-packages.md
Show resolved Hide resolved text/0000-ember-data-packages.md
@igorT

This comment has been minimized.

Member

igorT commented Nov 14, 2018

Need to add RecordData in

@igorT

This comment has been minimized.

Member

igorT commented Nov 14, 2018

Should add a discussion of moving the package into a separate github org

@runspired runspired changed the title from [WIP] @ember-data packages to @ember-data packages Nov 15, 2018

runspired added some commits Nov 15, 2018

sp

@runspired runspired referenced this pull request Nov 15, 2018

Open

Epic - improving Ember Data sections #20

0 of 9 tasks complete

runspired added some commits Nov 16, 2018

Merge pull request #1 from acorncom/patch-1
Adding proper @ember-data package name where missing

@amyrlam amyrlam referenced this pull request Nov 16, 2018

Merged

The Ember Times - Issue No. 74 #3696

5 of 13 tasks complete

runspired added some commits Nov 24, 2018

runspired added some commits Nov 26, 2018

@igorT

This comment has been minimized.

Member

igorT commented Nov 27, 2018

Action item from an in person discussion: IM implementation should go into the store package and Model should reexport it

@runspired

This comment has been minimized.

Contributor

runspired commented Nov 28, 2018

@igorT I don't think it's necessary to make that a detail of the RFC, only to note it for the initial implementation.

@igorT

This comment has been minimized.

Member

igorT commented Nov 28, 2018

From the ED meeting:

  • Move the org movement into a separate discussion
  • Add ember-data package part
  • Add migration path details/user impact
  • Maybe add ember-data/adapter, ember-data/serializer
  • @igorT to propose serializer/adapter splits
  • constraint: import paths need to match the local paths on disks:
    • complex discussion about package.json and nested paths
    • ember object has an example
  • bring in learning team

runspired added some commits Nov 29, 2018

@runspired

This comment has been minimized.

Contributor

runspired commented Nov 30, 2018

The most recent update to this RFC re-arranged imports to account for the no-double-slash problem. A short summary of this problem is this:

The node resolution algorithm problem

  • For better Editor and Typescript support, we want our modules to be resolvable using the node module resolution algorithm.
  • We want these things for both end consumers and maintainers of ember-data
  • Node only allows you to specify a main file, not a main directory, so anything not exported by the main file would need to "live" in the root of the package, or we would have to have users include src/ in their import path.
  • Workarounds to this include a publish-time move of files to the top level, or a publish time creation of package.json files that point to other modules from the top level (e.g. @ember-data/serializers/rest would be made available as the main file of @ember-data/serializers/rest/package.json
  • It is unclear whether any workaround would provide the same editor and Typescript support experience to maintainers as it would to end consumers

What our update did

Our update:

  1. decided to keep transforms in @ember-data/transforms instead of @ember-data/serializers/tranforms
  2. moved adapter errors from @ember-data/adapters/errors to @ember-data/adapter-errors
  3. moved the EmbeddedRecordsMixin from @ember-data/serializers/rest/embedded-records-mixin to @ember-data/embedded-records
  4. moved the BuildURLMixin from @ember-data/adapters/build-url-mixin to import { BuildURLMixin } from '@ember-data/adapters'
  5. moved all modules we wish to deprecate access to into a single -private module for working around this issue more easily (e.g import { ManyArray } from '@ember-data/model/-private';)
  6. moved individual adapters/serializers up a level into their primary package main file. e.g. (import Serializer from "@ember-data/serializers/rest"; became import { RESTSerializer } from "@ember-data/serializers";)

Some of these decisions are less than ideal and highlight what is likely to be a common struggle as the ember ecosystem seeks to standardize on a resolution algorithm.

What we lose

Consider the new import suggestion for the @ember-data/adapters module:

import Adapter, {
  RESTAdapter,
  JSONAPIAdapter,
  BuildURLMixin
} from '@ember-data/adapters';

A few things stand out here.

  1. Our mixin now has the same precedence as the reference adapter implementations.
  2. Users must import adapters with complicated to spell names. (before, import Adapter from '@ember-data/adapters/json-api' allowed users to name the import however seemed best).

This issue is also seen with the reference serializers:

import Serializer, {
  JSONSerializer
  RESTSerializer,
  JSONAPISerializer
} from '@ember-data/serializers';

We lose flexibility and risk being unnecessarily verbose

Flat, single entry packages are forced into decisions they shouldn't need to make.

In today's conventions, users typically

import Component from '<addon-name>/components/component-name';

While the oddly spelled Adapter and Serializer implementations here may not seem too big of a deal in isolation, consider if we were a ui component addon needing to export all of our components from a single module entry point.

We lose context

Import paths convey information to the user about the context of how something should be used.

From this perspective, two things this RFC initially set out to make more clear are lost.

  1. the embedded-records-mixin is meant for use with the rest serializer. This would have been conveyed by it being underneath the rest serializer and not serializers in general (@ember-data/serializers/rest/embedded-records-mixin)
  2. transforms are a fairly narrowly scoped feature of the reference serializer implementations, and not something with a wider use or purpose in ember-data (and not even one with very many guarantees). This would have been conveyed via their being in the submodule: @ember-data/serializers/transforms

We lose precedence

How code is organized speaks volumes to it's importance within an architecture and formulates the mental model and learning story.

transforms and adapter-errors for instance are implementation details of the provided reference serializers and adapters, they don't have the same status as a "core primitive" as say Model or RecordData or Store.

Basically these packages:

  • @ember-data/transforms
  • @ember-data/adapter-errors
  • @ember-data/embedded-records-mixin

Belong being sub-packages of @ember-data/adapters and @ember-data/serializers, not top level packages. But this issue encourages flattening to more top level packages. This makes the learning story less-focused.

@tomdale

This comment has been minimized.

Member

tomdale commented Nov 30, 2018

I don't think the naming rules for Ember Data should deviate from the rules used for Ember.js in RFC #176.

@chriskrycho

This comment has been minimized.

chriskrycho commented Nov 30, 2018

FWIW, I strongly agree with @tomdale here and I think if we have some set of limitations imposed by Node then we should work around Node's limitations.

However, it's worth note that it's certainly possible to support non-Node-friendly lookup rules (even with the "moduleResolution": "node" value set in tsconfig.json) in an application – we're doing that in our app today with my True Myth library, which does have nested module exports, not just at the root level:

import Maybe, { map } from 'true-myth/maybe';

☝️ is not a legal Node import (and accordingly there are workarounds in the library and notes in its docs for Node-only consumers), but it is a resolvable import in contexts which understand ES modules, and for all the reasons that RFC #176 hashed out, it works extremely well to support the design of module APIs that express the actual intent and design of the library.

@chriskrycho

This comment has been minimized.

chriskrycho commented Dec 5, 2018

An addendum to the above after I chatted with @runspired elsewhere for a while: one of the goals here is to make it so that the custom shenanigans I do to make this "just work" in the current Ember CLI pipeline is not necessary. And the base Node algorithm, as noted in that RFC, doesn't support it. That doesn't mean we shouldn't do the workarounds necessary, but it would be a lot nicer not to have to.

The net of that conversation was me feeling 🤔 🧐 about the right way forward here.

@runspired

This comment has been minimized.

Contributor

runspired commented Dec 5, 2018

cc @chriskrycho

one of the goals here is to make it so that the custom shenanigans I do to make this "just work" in the current Ember CLI pipeline is not necessary.

Precisely. A key constraint is that both maintainers and consumers of ember-data should not need to implement work arounds to have first-class typescript and editor support. There are tradeoffs, and these concerns are being raised just to make sure we properly take them into account.


cc @tomdale

I don't think the naming rules for Ember Data should deviate from the rules used for Ember.js in RFC #176.

Putting aside the conversation surrounding editor / typescript support, (e.g. in a perfect world in which those constraints were fully solved), ember-data still faces unique constraints that Ember did not have to consider when implementing the Ember Modules RFC. Here, I believe we can escape with merely following the spirit of the law, and not implementing it to the rule.

The Spirit

Guiding Learners
We can group framework classes and utilities by functionality, making it clear what things are related > and how they should work together. People can feel confident they are getting only what they need
at that moment, not an entire framework that they're not sure they're benefiting from.

For that reason, package names should assist the developer in understanding what capabilities are added by bringing in that new package.

(source)

Furthermore, the original RFC acknowledges that it may not account for all the necessary constraints

And keep in mind, this RFC specifies a baseline module API. Nothing here precludes adding additional models in the future, as we discover missing pieces.

Most importantly, the mental model was the driving principle behind the organization proposed by the RFC

This proposal attempts to re-align module naming with that simplified mental model, placing everything into packages based on the chunk of functionality they provide:

This organization was also driven by two rules:

If we organize by mental model, these rules conflict when put into practice in ember-data.

Specifically, AdapterError and it's subclasses are classes meant for use with Adapters. Transform and it's subclasses are classes meant for use with the reference Serializer implementations.

Using the default export rule and completely ignoring the one level deep requirement, organizing by the mental model could result in the following:

*note: mixins are treated as classes below, with the -mixin addition to the name to avoid an extra /

import Adapter from '@ember-data/adapter';
import BuildURL from '@ember-data/adapter/build-url-mixin';
import Adapter from '@ember-data/adapter/rest';
import Adapter from '@ember-data/adapter/json-api';
import AdapterError, { errorsHashToArray, errorsArrayToHash } from '@ember-data/adapter/error';
import InvalidError from '@ember-data/adapter/error/invalid';
import TimeoutError from '@ember-data/adapter/error/timeout';
import AbortError from '@ember-data/adapter/error/abort';
import UnauthorizedError from '@ember-data/adapter/error/unauthorized';
import ForbiddenError from '@ember-data/adapter/error/forbidden';
import NotFoundError from '@ember-data/adapter/error/not-found';
import ConflictError from '@ember-data/adapter/error/conflict';
import ServerError from '@ember-data/adapter/error/server';
//...
import Serializer from '@ember-data/serializer';
import Serializer from '@ember-data/serializer/json';
import Serializer from '@ember-data/serializer/rest';
import EmbeddedRecords from '@ember-data/serializer/rest/embedded-records-mixin';
import Serializer from '@ember-data/serializer/json-api';
import Transform from '@ember-data/serializer/transform';
import DateTransform from '@ember-data/serializer/transform/date';
import BooleanTransform from '@ember-data/serializer/transform/boolean';
import NumberTransform from '@ember-data/serializer/transform/number';
import StringTransform from '@ember-data/serializer/transform/string';

What this shows is that a strict interpretation of the classes are default exports condition is incompatible with the one level deep condition when organizing according to the mental model.

However

There's also the following considerations here that Ember does not really have:

  • sub-classes (multiple) of primary classes
  • sub-classes that will almost never be directly imported (transforms) due to being automatically registered in a manner such that they may be "resolved"

With this in mind, we could potentially make the argument that sub-classes could be "named exports". In the case of Errors this follows the intuitive naming of these classes anyway (InvalidError TimeoutError etc.), but it does mean we end up with Transform and Adapter in the names of the reference implementations of adapters, serializers, and transforms. This is a tradeoff we may just want to accept: there is an outlier in the case of embedded-records though, which results in a potential move it to it's own package.

import Adapter, {
  RESTAdapter,
  JSONApiAdapter,
  BuildURLMixin
} from '@ember-data/adapter';
import AdapterError, {
  errorsHashToArray,
  errorsArrayToHash,
  InvalidError,
  TimeoutError,
  AbortError,
  UnauthorizedError,
  ForbiddenError,
  NotFoundError,
  ConflictError,
  ServerError
} from '@ember-data/adapter/error';
//...
import Serializer, {
  JSONSerializer,
  RESTSerializer,
  JSONAPISerializer
} from '@ember-data/serializer';
import EmbeddedRecords from '@ember-data/embedded-records-mixin';
import Transform, {
  DateTransform,
  BooleanTransform,
  NumberTransform,
  StringTransform
} from '@ember-data/serializer/transform';

In some cases, this is a nice cleanup, but in others it produces awkwardness.

There is still a third option, a middle ground, in which we recognize the increased importance of Adapter and Serializer and their relative Prominence in the mental model by keeping their subclasses as default exports. Personally, I find this the most intuitive, but it must be conceded that the "mixed" nature of this approach risks confusion... but likely no more confusion than "sub classes not being default exports" already risks.

import Adapter, { BuildURL } from '@ember-data/adapter';
import Adapter from '@ember-data/adapter/rest';
import Adapter from '@ember-data/adapter/json-api';
import AdapterError, {
  errorsHashToArray,
  errorsArrayToHash,
  InvalidError,
  TimeoutError,
  AbortError,
  UnauthorizedError,
  ForbiddenError,
  NotFoundError,
  ConflictError,
  ServerError
} from '@ember-data/adapter/error';
//...
import Serializer from '@ember-data/serializer';
import Serializer from '@ember-data/serializer/json';
import Serializer, { EmbeddedRecords } from '@ember-data/serializer/rest';
import Serializer from '@ember-data/serializer/json-api';
import Transform, {
  DateTransform,
  BooleanTransform,
  NumberTransform,
  StringTransform
} from '@ember-data/serializer/transform';

It's been quite some time since I came to the realization that regardless of approach, we're weighing inescapable tradeoffs and making compromises when it comes to the adapter and serializer package organization. The main positive is that this split into packages is the first step towards providing a world in which these packages aren't necessary at all, and whatever compromises made here might end here too.

@bmac

bmac approved these changes Dec 5, 2018

@tomdale

This comment has been minimized.

Member

tomdale commented Dec 5, 2018

I don't understand the concern about IDE/editor integration. Can someone unpack "custom build shenanigans"?

The Node module resolution algorithm absolutely supports nested modules in npm packages, in the form @scope/pkg/module, natively, without any additional configuration needed. You can ship packages like this as static files to npm and they will work just fine with TypeScript, Node, webpack, Rollup, or anything that uses the Node module resolution algorithm under the hood.

See https://docs.google.com/document/d/1CZC2rcpxffTDfRDs6p1cfbmKNLA6x5O-NtkJglDaBVs/preview#heading=h.k0mh3o8u5hx for a description of how Angular handles structuring their packages to accomplish this natively.

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