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

[FEAT identifiers] adds flagged public API and integration tests #6272

Merged
merged 4 commits into from
Jul 26, 2019
Merged

[FEAT identifiers] adds flagged public API and integration tests #6272

merged 4 commits into from
Jul 26, 2019

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Jul 22, 2019 •
edited
Loading

resolves #6170
closes #6171

Duplicate records on save (mostly already closed in favor of 1829)
resolves #1726
resolves #1829
resolves #4262
resolves #5083
resolves #5237
resolves #4972
resolves adopted-ember-addons/ember-data-model-fragments#221
resolves emberjs/rfcs#173
resolves #4552

STI / Polymorphism
resolves #2407
resolves #4377

Forum
resolves https://discuss.emberjs.com/t/proposal-fix-saving-new-embedded-records-creates-duplicates/3677/17

Once relationships are refactored such that they are managed by identifier as well there's another long list to close :)

@runspired runspired changed the title [WIP FEAT identifiers] adds flagged public API and integration tests [FEAT identifiers] adds flagged public API and integration tests Jul 23, 2019
@runspired runspired requested review from igorT and hjdivad July 23, 2019 23:51
@runspired runspired mentioned this pull request Jul 24, 2019
Closed
@runspired runspired force-pushed the feat/identifier-tests branch from 01f583e to 24bab59 Compare July 24, 2019 01:13
@runspired runspired mentioned this pull request Jul 24, 2019
Merged
runspired
runspired commented Jul 25, 2019
View reviewed changes
packages/-ember-data/tsconfig.json Show resolved Hide resolved
packages/canary-features/addon/default-features.js Show resolved Hide resolved
packages/store/addon/-private/identifiers/cache.ts Show resolved Hide resolved
packages/store/addon/-private/identifiers/cache.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/identifiers/cache.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/store/internal-model-factory.ts Show resolved Hide resolved
packages/store/addon/-private/system/store/record-data-store-wrapper.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/ts-interfaces/ember-data-json-api.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/ts-interfaces/identifier.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/utils/has-valid-id.ts Show resolved Hide resolved
igorT
igorT approved these changes Jul 25, 2019
View reviewed changes
Copy link
Member

@igorT igorT left a comment

Choose a reason for hiding this comment

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

reviewed under @runspired account. seems good after comments addressed and @mike-north 👍

@runspired runspired force-pushed the feat/identifier-tests branch 3 times, most recently from 9495201 to 5937fc4 Compare July 25, 2019 05:08
@mike-north
Copy link
Contributor

still working on reviewing this

mike-north
mike-north suggested changes Jul 26, 2019
View reviewed changes
packages/-ember-data/tests/integration/identifiers/lid-reflection-test.ts Show resolved Hide resolved
packages/-ember-data/tests/integration/identifiers/configuration-test.ts Show resolved Hide resolved
packages/-ember-data/tests/integration/identifiers/configuration-test.ts Show resolved Hide resolved
packages/-ember-data/tests/integration/identifiers/configuration-test.ts Show resolved Hide resolved
packages/-ember-data/tests/integration/identifiers/configuration-test.ts Show resolved Hide resolved
packages/-ember-data/tests/integration/identifiers/configuration-test.ts Show resolved Hide resolved
packages/-ember-data/tests/integration/identifiers/lid-reflection-test.ts Show resolved Hide resolved
packages/-ember-data/tests/integration/identifiers/scenarios-test.ts
let store;
let calls;
let secondaryCache: {
id: Dict<string, string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

When did the Dict type get changed to allow two typeparams? ({ [KK in K]: V; }). It's now not really a dictionary (arbitrary keys) and matches nearly exactly with the built-in Record<K, V> utility type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch for two reasons:

  1. I was not aware of Record<K, V> being a built-in type and that explains why I've sometimes found that definition trying to trump our own defined Record type.

  2. As far as I can recall this was the Dict we introduced together a while back. I'll make a cleanup pass to refactor to Dict<ValueType>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(said cleanup pass will be separate from this PR as Dict has this <K, V>` usage more broadly)

packages/store/addon/-private/system/coerce-id.ts Show resolved Hide resolved
packages/store/addon/-private/system/store/record-data-store-wrapper.ts
if (internalModel) {
internalModel.destroyFromRecordData();
}
}
}

function hasValidId(id?: string | null, clientId?: string | null): id is string {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a questionable type guard, and if misused could result in some "lying" in the type-checker. There's a significant difference between the static analysis behavior

"true if id is a string"

and the runtime behavior

"true unless both id and clientId are truthy"

We should take a close look at this before merging the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would also note that the near-term solution for not needing this craziness is to continue refactoring towards use of identifiers. Because identifiers are typed as a union of these two cases (one or the other must be a string) we reduce the number of method overloads and pass the "overloaded" type signature around with the value instead.

@runspired runspired force-pushed the feat/identifier-tests branch from 5937fc4 to 2028505 Compare July 26, 2019 21:47
@runspired
Copy link
Contributor Author

runspired commented Jul 26, 2019 •
edited
Loading

2 test failures to investigate, only one blocking

  • blocking: need to flag one new test as being dev-time only (FIXED)
  • investigate: when all feature-flags are one, one identifiers test fails. unsure which other flag is causing this, but shouldn't block. (FIXED) the offending flag was RECORD_DATA_STATE and the issue was that isNew is set to false the moment a canonical payload is received when using derived state where previously we would have waited for the pending save to resolve.

@runspired runspired merged commit b0cf322 into master Jul 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/identifier-tests branch July 26, 2019 22:59
@runspired runspired mentioned this pull request Jul 27, 2019
Merged
@sandstrom
Copy link
Contributor

Fantastic @runspired! ❤️

@hjdivad hjdivad mentioned this pull request Aug 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mike-north mike-north mike-north requested changes

@igorT igorT igorT approved these changes

@hjdivad hjdivad Awaiting requested review from hjdivad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@runspired @mike-north @sandstrom @igorT