[Chore]: TypeScript adapters #7358
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Performance Report for 98784c5 Relationship Analysis |
|
Asset Size Report for 98784c5 IE11 Builds The size of the library EmberData has increased by 576.0 B (124.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsThe uncompressed size of the package @ember-data/adapter has increased by 576.0 B. Changeset Full Asset Analysis (IE11)Modern Builds The size of the library EmberData has increased by 576.0 B (124.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsThe uncompressed size of the package @ember-data/adapter has increased by 576.0 B. Changeset Full Asset Analysis (Modern)Modern Builds (No Rollup) The size of the library EmberData has increased by 652.0 B (42.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsThe uncompressed size of the package @ember-data/adapter has increased by 652.0 B. Changeset Full Asset Analysis (Modern) |
snewcomer
force-pushed
the
sn/ts-adapters
branch
from
64f879c
to
3047ee3
Compare
snewcomer
commented
Nov 5, 2020
| @@ -309,14 +320,15 @@ class RESTAdapter extends Adapter.extend(BuildURLMixin) { | |||
| } | |||
|
|
|||
| set fastboot(value) { | |||
| return (this._fastboot = value); | |||
| this._fastboot = value; | |||
There was a problem hiding this comment.
We could take this private property off the class if you so desired with a WeakMap
snewcomer
force-pushed
the
sn/ts-adapters
branch
2 times, most recently
from
c61644a
to
fd1f69d
Compare
snewcomer
commented
Nov 11, 2020
packages/adapter/addon/index.ts
Outdated
| @@ -113,6 +122,13 @@ export default class Adapter extends EmberObject { | |||
| @param {Snapshot} snapshot | |||
| @return {Promise} promise | |||
| */ | |||
| findRecord(store: Store, type: ShimModelClass, id: string, snapshot: Snapshot) { | |||
| warn('You subclassed the Adapter class but missing a findRecord override', false, { | |||
There was a problem hiding this comment.
OR something like
You subclassed the Adapter class and have not defined a 'findRecord' method. Please implement 'findRecord(store: Store, type: ShimModelClass, id: string, snapshot: Snapshot) -> Promise<unknown>'
| return { | ||
| status: response.status, | ||
| textStatus: response.textStatus, | ||
| textStatus: response.statusText, |
There was a problem hiding this comment.
igorT
reviewed
Nov 24, 2020
| buildURL(modelName, id, snapshot, requestType, query) { | ||
| buildURL( | ||
| modelName: string, | ||
| id: string | string[] | Record<string, any> | null, |
There was a problem hiding this comment.
The Record would be very confusing for the data repo, so we use Dict and ConfidentDict instead
igorT
reviewed
Nov 24, 2020
| @@ -89,9 +98,9 @@ export default Mixin.create({ | |||
| @param {String} id | |||
| @return {String} url | |||
| */ | |||
| _buildURL(modelName, id) { | |||
| _buildURL(modelName: string, id: string): string { | |||
There was a problem hiding this comment.
In the code here it looks like id can be null/undefined?
igorT
reviewed
Nov 24, 2020
| @@ -195,7 +204,7 @@ export default Mixin.create({ | |||
| @param {String} modelName | |||
| @return {String} url | |||
| */ | |||
| urlForQuery(query, modelName) { | |||
| urlForQuery(query: Record<string, any>, modelName: string): string { | |||
igorT
reviewed
Nov 24, 2020
| @@ -388,7 +397,7 @@ export default Mixin.create({ | |||
| @param {String} parentURL | |||
| @return {String} urlPrefix | |||
| */ | |||
| urlPrefix(path, parentURL) { | |||
| urlPrefix(path: string, parentURL: string): string { | |||
There was a problem hiding this comment.
path can be null/undefined/'' by reading the code
igorT
reviewed
Nov 24, 2020
| export interface Request { | ||
| method: string; | ||
| body: unknown; | ||
| cookies: unknown; |
There was a problem hiding this comment.
Lets have a bit more specific types here for objects
There was a problem hiding this comment.
What do you have in mind here?
Re: cookies...
An interface for headers I guess could include a few like Content-Type and the like with a catch all. Is this the idea?
There was a problem hiding this comment.
Just to confirm, you generally want only the used properties declared as a part of this interface?
igorT
reviewed
Nov 24, 2020
packages/adapter/addon/index.ts
Outdated
| @@ -580,7 +638,7 @@ export default class Adapter extends EmberObject { | |||
| @param {SnapshotRecordArray} snapshotRecordArray | |||
| @return {Boolean} | |||
| */ | |||
| shouldReloadAll(store, snapshotRecordArray) { | |||
| shouldReloadAll(store: Store, snapshotRecordArray) { | |||
igorT
reviewed
Nov 24, 2020
packages/adapter/addon/json-api.ts
Outdated
| @@ -156,7 +160,7 @@ class JSONAPIAdapter extends RESTAdapter { | |||
| @param {Object} options | |||
| @return {Object} | |||
| */ | |||
| ajaxOptions(url, type, options = {}) { | |||
| ajaxOptions(url: string, type: string, options = {}) { | |||
igorT
reviewed
Nov 24, 2020
packages/adapter/addon/rest.ts
Outdated
| findAll(store, type, sinceToken, snapshotRecordArray) { | ||
| let query = this.buildQuery(snapshotRecordArray); | ||
| findAll(store: Store, type: ShimModelClass, sinceToken, snapshotRecordArray: SnapshotRecordArray) { | ||
| let query: Record<string, any> = this.buildQuery(snapshotRecordArray); |
igorT
reviewed
Nov 24, 2020
packages/adapter/addon/rest.ts
Outdated
| @@ -578,7 +588,7 @@ class RESTAdapter extends Adapter.extend(BuildURLMixin) { | |||
| @param {Object} query | |||
| @return {Promise} promise | |||
| */ | |||
| queryRecord(store, type, query) { | |||
| queryRecord(store: Store, type: ShimModelClass, query) { | |||
igorT
reviewed
Nov 24, 2020
packages/adapter/addon/rest.ts
Outdated
| @@ -662,7 +672,7 @@ class RESTAdapter extends Adapter.extend(BuildURLMixin) { | |||
| @param {Object} relationship meta object describing the relationship | |||
| @return {Promise} promise | |||
| */ | |||
| findHasMany(store, snapshot, url, relationship) { | |||
| findHasMany(store: Store, snapshot: Snapshot, url: string, relationship) { | |||
igorT
reviewed
Nov 24, 2020
packages/adapter/addon/rest.ts
Outdated
| @@ -550,7 +560,7 @@ class RESTAdapter extends Adapter.extend(BuildURLMixin) { | |||
| @param {Object} query | |||
| @return {Promise} promise | |||
| */ | |||
| query(store, type, query) { | |||
| query(store: Store, type: ShimModelClass, query) { | |||
There was a problem hiding this comment.
We should type a return value for all of these
igorT
reviewed
Nov 24, 2020
packages/adapter/addon/rest.ts
Outdated
| let url = this.buildURL(snapshot.modelName, snapshot.id, snapshot); | ||
|
|
||
| let expandedURL = url.split('/'); | ||
| let expandedURL: string[] = url.split('/'); |
igorT
reviewed
Nov 24, 2020
packages/adapter/addon/rest.ts
Outdated
| if (this.isSuccess(status, headers, payload)) { | ||
| return payload; | ||
| } else if (this.isInvalid(status, headers, payload)) { | ||
| return new InvalidError(payload.errors); | ||
| return new InvalidError((payload as Record<string, any>).errors); |
igorT
reviewed
Nov 24, 2020
packages/adapter/addon/rest.ts
Outdated
| @@ -1206,8 +1215,8 @@ class RESTAdapter extends Adapter.extend(BuildURLMixin) { | |||
| @param {Snapshot} snapshot | |||
| @return {Object} | |||
| */ | |||
| buildQuery(snapshot) { | |||
| let query = {}; | |||
| buildQuery(snapshot: Snapshot | SnapshotRecordArray): object { | |||
There was a problem hiding this comment.
Can we have a better return type here
igorT
reviewed
Nov 24, 2020
packages/adapter/addon/rest.ts
Outdated
| @@ -501,7 +511,7 @@ class RESTAdapter extends Adapter.extend(BuildURLMixin) { | |||
| @param {Snapshot} snapshot | |||
| @return {Promise} promise | |||
| */ | |||
| findRecord(store, type, id, snapshot) { | |||
| findRecord(store: Store, type: ShimModelClass, id: string, snapshot: Snapshot) { | |||
snewcomer
force-pushed
the
sn/ts-adapters
branch
2 times, most recently
from
01a0164
to
2d6b3d9
Compare
igorT
reviewed
Dec 9, 2020
igorT
reviewed
Dec 9, 2020
…SON.parse undefined or emtpy string
snewcomer
force-pushed
the
sn/ts-adapters
branch
from
124deef
to
759c470
Compare
25 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
follow up #7350