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

[CHORE] redivide assets to move record-data into package #6513

Merged
merged 7 commits into from
Oct 2, 2019

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Sep 26, 2019

Types are hard :(

A few notes:

TS Config Pains

  • whatever package you open first in vscode is the tsconfig.json that applies to the rest
  • the -ember-data tsconfig.json is what is used when running tests and applied to all packages
  • running tsc (as we do during CI and release) correctly uses the appropriate tsconfig.json for each package

This is gnarly and leads to a horrible experience. We should refactor to a single root tsconfig.json and teach ember-cli-typescript how to find it.

Circular Types & Imports

  • some interfaces were refactored and a new technique for type import was introduced to handle issues around circular type imports between the store and record-data packages. As we further refactor away from internal-model we should be able to eliminate much of this weirdness.
  • a few utils are now duplicated in record-data / store to avoid circular imports (relationshipStateFor coerceId)

DefaultRecordData

We now wire RecordData in via the ember-data/store for apps consuming ember-data. "BYOPackages" mode requires doing the same.

@runspired runspired force-pushed the feat/record-data-package branch from f79a5f7 to 22c41ee Compare September 27, 2019 00:50
@runspired runspired added the 🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166 label Sep 27, 2019
packages/-ember-data/addon/store.js Outdated Show resolved Hide resolved
.eslintignore Outdated Show resolved Hide resolved
packages/-ember-data/addon/store.ts Show resolved Hide resolved
packages/-ember-data/tsconfig.json Show resolved Hide resolved
packages/record-data/types/@ember/runloop/index.d.ts Outdated Show resolved Hide resolved
@runspired runspired force-pushed the feat/record-data-package branch 2 times, most recently from e66cffb to 960924f Compare September 28, 2019 01:29
@chriskrycho
Copy link
Contributor

I'm happy to help tweak the experience for making sure it works well for VS Code – it'll be relatively straightforward, and it'll be useful prior art for other projects I'm working on. The two approaches that should Just Work™ are:

  • Create VS Code workspace and open that instead of opening individual packages.
  • Create a root tsconfig.json and have the other tsconfig.json files extend from it. The bits that matter for ember-cli-typescript should all still work as expected, since the specific mappings it sets up are not used for transpilation, only for type resolution.

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Had a couple catches and a bunch of questions.

I also have one possible step toward working around the current issues with recordDataFor:

I have a type workaround that makes recordDataFor work, and if we adopt it we'll probably want to use its workaround in these types as well. Supplying that workaround will almost certainly catch bugs – it did catch them in core-store.ts.

recordDataFor workaround implementation
import RecordData from "../ts-interfaces/record-data";

// These types exist to support the backwards-compatible, "loose" lookup below.
type WithPrivateInternalModel = { _internalModel: { _recordData?: RecordData } };
type WithPublicInternalModel = { internalModel: { _recordData?: RecordData } };
type WithRecordData = { _recordData?: RecordData }

type Instance =
  | WithPrivateInternalModel
  | WithPublicInternalModel
  | WithRecordData;

/*
 * Returns the RecordData instance associated with a given
 * Model or InternalModel.
 *
 * Intentionally "loose" to allow anything with an _internalModel
 * property until InternalModel is eliminated.
 *
 * Intentionally not typed to `InternalModel` due to circular dependency
 *  which that creates.
 *
 * Overtime, this should shift to a "weakmap" based lookup in the
 *  "Ember.getOwner(obj)" style.
 */
export default function recordDataFor(instance: Instance) {
  let internalModel =
    (instance as WithPrivateInternalModel)._internalModel ||
    (instance as WithPublicInternalModel).internalModel ||
    instance;

  return internalModel._recordData || null;
}
errors in core-store.ts
../store/addon/-private/system/core-store.ts:1630:44 - error TS2531: Object is possibly 'null'.

1630             data: internalModels.map(im => recordDataFor(im).getResourceIdentifier()),
                                                ~~~~~~~~~~~~~~~~~

../store/addon/-private/system/core-store.ts:1630:62 - error TS2339: Property 'getResourceIdentifier' does not exist on type 'RecordData'.

1630             data: internalModels.map(im => recordDataFor(im).getResourceIdentifier()),
                                                                  ~~~~~~~~~~~~~~~~~~~~~

../store/addon/-private/system/core-store.ts:1708:41 - error TS2531: Object is possibly 'null'.

1708         let response = internalModel && recordDataFor(internalModel).getResourceIdentifier();
                                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

../store/addon/-private/system/core-store.ts:1708:70 - error TS2339: Property 'getResourceIdentifier' does not exist on type 'RecordData'.

1708         let response = internalModel && recordDataFor(internalModel).getResourceIdentifier();
                                                                          ~~~~~~~~~~~~~~~~~~~~~

../store/addon/-private/system/core-store.ts:3101:5 - error TS2322: Type 'RecordData | null' is not assignable to type 'RecordData'.
  Type 'null' is not assignable to type 'RecordData'.

3101     return recordDataFor(internalModel);
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

At a minimum, even if it's not possible to adopt that type (and expansions of it in calling contexts) for now, you may be able to use it to chase down possible source of bugs, as in the cases I caught here.

fix recordDataFor

unentangle createRecordData

creatively un-entangle the store from record-data

DS.Store needs to also use the configured defaultRecordData

none of record-data package should be JS :)

make types for record-data package nice
@runspired runspired force-pushed the feat/record-data-package branch from 3fc97e4 to 59acc1b Compare October 1, 2019 20:59
Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

:shipit:

@runspired runspired merged commit 681d922 into master Oct 2, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/record-data-package branch October 2, 2019 00:34
Copy link
Contributor

@Gaurav0 Gaurav0 left a comment

Choose a reason for hiding this comment

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

Applications shouldn't have to know about these sorts of details of ember-data.

igorT added a commit that referenced this pull request Jan 12, 2020
Dividing Record Data into packages (#6513) configured the Store class
inside of the `-ember-data` metapackage. This broke importing the Store directly from `@ember-data/store`,
which this commit fixes.
igorT added a commit that referenced this pull request Jan 12, 2020
Dividing Record Data into packages (#6513) configured the Store class
inside of the `-ember-data` metapackage. This broke importing the Store directly from `@ember-data/store`,
which this commit fixes.
hjdivad added a commit that referenced this pull request Jan 24, 2020
Dividing Record Data into packages (#6513) configured the St ore class
inside of the `-ember-data` metapackage. This broke importing the Store directly from `@ember-da ta/store`,
which this commit fixes.

(cherry-picked from commit 3e3677f)
hjdivad pushed a commit that referenced this pull request Jan 24, 2020
Dividing Record Data into packages (#6513) configured the St ore class
inside of the `-ember-data` metapackage. This broke importing the Store directly from `@ember-da ta/store`,
which this commit fixes.

(cherry-picked from commit 3e3677f)
hjdivad pushed a commit that referenced this pull request Jan 24, 2020
Dividing Record Data into packages (#6513) configured the Store class
inside of the `-ember-data` metapackage. This broke importing the Store directly from `@ember-data/store`,
which this commit fixes.

(cherry picked from commit 3e3677f)
hjdivad pushed a commit that referenced this pull request Jan 24, 2020
Dividing Record Data into packages (#6513) configured the St ore class
inside of the `-ember-data` metapackage. This broke importing the Store directly from `@ember-da ta/store`,
which this commit fixes.

(cherry-picked from commit 3e3677f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants