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

fixes #28 and #32 #35

Closed
wants to merge 2 commits into from
Closed

fixes #28 and #32 #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c3f2b1
fixes #28
capaj May 24, 2018
33cb7ba
fixes #32
capaj May 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/domains/field/compiler/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ export function compileFieldConfig(
target: Function,
fieldName: string,
): GraphQLFieldConfig<any, any, any> {
const { type, description, isNullable } = fieldsRegistry.get(
const { type, description, isNullable, isGetter } = fieldsRegistry.get(
target,
fieldName,
);
const args = compileFieldArgs(target, fieldName);

const args = compileFieldArgs(target, fieldName);
const resolvedType = resolveRegisteredOrInferedType(target, fieldName, type);

// if was not able to resolve type, try to show some helpful information about it
Expand All @@ -41,7 +41,7 @@ export function compileFieldConfig(
return {
description,
type: finalType,
resolve: compileFieldResolver(target, fieldName),
resolve: compileFieldResolver(target, fieldName, isGetter),
args,
};
}
Expand Down
15 changes: 13 additions & 2 deletions src/domains/field/compiler/resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ function computeFinalArgs(
export function compileFieldResolver(
target: Function,
fieldName: string,
isGetter: boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can it be replaced in favor of using Object.getOwnPropertyDescriptor to detect it automatically? Passing it over like this might make it harder to maintain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it could, but doing Object.getOwnPropertyDescriptor is performance costly. This might sound like a micro optimisation, but it's not. See for example chaijs/chai#127 (comment).

Having it detected only once is worth the faster performance IMHO. Decorators always run statically at startup, so it doesn't make sense to support a scenario where someone would change a property into a getter at runtime.

): GraphQLFieldResolver<any, any> {
// const config = fieldsRegistry.get(target, fieldName);
const injectors = injectorRegistry.getAll(target)[fieldName];
Expand All @@ -70,8 +71,18 @@ export function compileFieldResolver(

return async (source: any, args = null, context = null, info = null) => {
await performHooksExecution(beforeHooks, source, args, context, info);
const instanceField =
(source && source[fieldName]) || target.prototype[fieldName];

if (isGetter) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole block is hard to understand for me. Could we make it external function that would abstract it away with proper name. It seems like the point here is to get target property.

const valueFromGetter = source[fieldName];
await performHooksExecution(afterHooks, source, args, context, info);
return valueFromGetter;
}
let instanceField;
if (source && source.hasOwnProperty(fieldName)) {
instanceField = source[fieldName];
} else {
instanceField = target.prototype[fieldName];
}

if (typeof instanceField !== 'function') {
await performHooksExecution(afterHooks, source, args, context, info);
Expand Down
6 changes: 6 additions & 0 deletions src/domains/field/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,13 @@ export function Field(options?: FieldOptions): PropertyDecorator {
...options,
};

const descriptor = Object.getOwnPropertyDescriptor(
targetInstance,
fieldName,
);

fieldsRegistry.set(targetInstance.constructor, fieldName, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it can be calculated from targetInstance, then passing it as another parameter is redundant in my opinion.

isGetter: descriptor && !!descriptor.get,
...finalConfig,
});
};
Expand Down
1 change: 1 addition & 0 deletions src/domains/field/registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export interface FieldInnerConfig {
property: string;
description?: string;
isNullable?: boolean;
isGetter?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm quite strongly against having it explicitly here. It looks like you might define it. But it's part of business logic that could propably be calculated 'on the fly' when needed.

Config is what user or system want it to be. Getter is implementation detail, so I'd say it's not part of config.

Copy link
Contributor Author

@capaj capaj May 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

like I've mentioned-detecting getter always using Object.getOwnPropertyDescriptor is quite costly.
I agree this should not be exposed to a client of this library like I did. I do think it's alright to pass down in a private config type. I'll try to refactor.

type?: any;
}

Expand Down
5 changes: 5 additions & 0 deletions src/services/utils/gql/types/parseNative.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,15 @@ export function inferTypeByTarget(target: Function, key?: string) {
}

const returnType = Reflect.getMetadata('design:returntype', target, key);

if (returnType) {
return returnType;
}

const descriptor = Object.getOwnPropertyDescriptor(target, key);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this function is already quite complicated and not obvious. Making external function like isGetter would make it more readable and obvious about what is the point here.

if (descriptor && descriptor.get) {
return Reflect.getMetadata('design:type', target, key);
}
const targetField = (target as any)[key];

if (targetField && typeof targetField === 'function') {
Expand Down
13 changes: 11 additions & 2 deletions src/test/field/getters.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,25 @@ import { ObjectType, Field, compileObjectType } from '~/domains';

describe('Fields based on getters', () => {
it('Will work with getter field', async () => {
let foo: Foo;
@ObjectType({ description: 'Simple product object type' })
class Foo {
a: number;
@Field()
get bar(): number {
return 42;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's good way to perform tests by throwing errors to stop them.

There might be different ways to accomplish this like adding instance field and then returning result like return this.foo and that should be enough to test if instance was avaliable

if (this !== foo) {
throw new Error(
'getter must be resolved with an instance as a context ',
);
}
return this.a + 21;
}
}

const { bar } = compileObjectType(Foo).getFields();
const resolvedValue = await bar.resolve(new Foo(), null, null, null);
foo = new Foo();
foo.a = 21;
const resolvedValue = await bar.resolve(foo, null, null, null);
expect(resolvedValue).toEqual(42);
expect(bar.type).toEqual(GraphQLFloat);
});
Expand Down
24 changes: 22 additions & 2 deletions src/test/field/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,31 @@ describe('Field', () => {
@ObjectType()
class Foo {
@Field() bar: string = 'baz';
@Field() undef: boolean = undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say this test is out of scope of resolving default value. It's rather another test case like Resolves with edge-case default values

@Field() b1: boolean = false;
@Field() b2: boolean = true;
@Field() n0: number = 0;
@Field() nMax: number = Number.MAX_SAFE_INTEGER;
}
const compiled = compileObjectType(Foo);
const barField = compiled.getFields().bar;

expect(await barField.resolve(new Foo(), {}, null, null)).toEqual('baz');
const barField = compiled.getFields().bar;
const undefField = compiled.getFields().undef;
const b1Field = compiled.getFields().b1;
const b2Field = compiled.getFields().b2;
const n0Field = compiled.getFields().n0;
const nMaxField = compiled.getFields().nMax;

const foo = new Foo();

expect(await barField.resolve(foo, {}, null, null)).toEqual('baz');
expect(await undefField.resolve(foo, {}, null, null)).toEqual(undefined);
expect(await b1Field.resolve(foo, {}, null, null)).toEqual(false);
expect(await b2Field.resolve(foo, {}, null, null)).toEqual(true);
expect(await n0Field.resolve(foo, {}, null, null)).toEqual(0);
expect(await nMaxField.resolve(foo, {}, null, null)).toEqual(
9007199254740991
);
});

it('Resolves fields with function resolver', async () => {
Expand Down