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

Fallbackid #8

Merged
merged 20 commits into from
Aug 8, 2018
Merged

Fallbackid #8

merged 20 commits into from
Aug 8, 2018

Conversation

Jontem
Copy link
Contributor

@Jontem Jontem commented Aug 7, 2018

Here is a WIP for fallback id

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 75.697% when pulling 1c6e99b on fallback-id into 350b575 on master.

@coveralls
Copy link

coveralls commented Aug 7, 2018

Coverage Status

Coverage increased (+1.0%) to 75.794% when pulling 2c869ea on fallback-id into 811529b on master.

@jonaskello
Copy link
Member

One point of discussion is the signature on the user supplied getObjectId. Now I guess it accepts an extra parameter for path but instead it could have the same signature as before and if it returns undefined then a generated path will be used. It would depend on if the user wants to access the path when he generates the path.

@Jontem
Copy link
Contributor Author

Jontem commented Aug 7, 2018

Yes that was my first idea. But I went with a non breaking solution for the first implementation.

I try to think of any possible use case for the implementor of getObjectId to use path but nothing comes to mind. Maybe we shouldn't expose it. This is the very last fallback so it makes sense to be in control of that.

src/types.ts Outdated
) => string;
},
path: ReadonlyArray<string>
) => string | undefined;
Copy link
Contributor Author

@Jontem Jontem Aug 7, 2018

Choose a reason for hiding this comment

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

@jonaskello maybe it would be clearer if getObjectToId returned a "resultobject" which could look like

export type ObjectToIdResult = ObjectToIdResultSuccess | ObjectToIdResultFailure

export interface ObjectToIdResultSuccess {
    readonly resolved: true;
    readonly id: string
}

export interface ObjectToIdResultFailure {
    readonly resolved: false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should have som performance tests what implications code changes has

@Jontem
Copy link
Contributor Author

Jontem commented Aug 8, 2018

I have tried different approaches regarding to performance but it is hard to get an reliable result. Sometimes the normalize is slower than denormalize, sometimes the other way around. But using less objects and more primitives should be faster so i changed path to be a string and GetObjectToId signature to be

export type GetObjectToIdResult = string | undefined;

export type GetObjectId = (
  object: {
    readonly id?: string;
    readonly __typename?: string;
  }
) => GetObjectToIdResult;

@Jontem Jontem merged commit 4220d7a into master Aug 8, 2018
@jonaskello
Copy link
Member

Regarding performance one idea could be to use benchmarkjs. Since benchmarking results may be different between rounds, I think this is mostly useful for comparing things. So one could make a script to compare the latest published version on npm to the current master version for example.

@Jontem
Copy link
Contributor Author

Jontem commented Aug 9, 2018

@jonaskello this is almost exactly what i did. I added benchmarkjs projects and have created a simple performance test. I just didn't get the package from npm, only tested with master branch and my feature branch. But the result was different from time to time. Sometimes the master branch was faster and sometimes the feature branch.

PS: I only did a test with node. No browsers

@jonaskello
Copy link
Member

Ok, well that's interesting anyway. Did you add those scripts to package.json?

@Jontem
Copy link
Contributor Author

Jontem commented Aug 9, 2018

Yes just run yarn performance

@jonaskello jonaskello deleted the fallback-id branch July 21, 2019 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants