Skip to content

Commit

Permalink
Remove RelayValidator
Browse files Browse the repository at this point in the history 
Reviewed By: kassens

Differential Revision: D17815205

fbshipit-source-id: d9a1fc7d5ee25b73bf16674b2b26a8c47ea36b9c
  • Loading branch information
@alunyov @facebook-github-bot
alunyov authored and facebook-github-bot committed Oct 10, 2019
1 parent 25c6769 commit 15e8d22
Show file tree
Hide file tree
Showing 8 changed files with 2 additions and 154 deletions.
20 changes: 0 additions & 20 deletions packages/relay-compiler/codegen/RelayFileWriter.js
View file
Expand Up @@ -69,10 +69,6 @@ export type WriterConfig = {
// Haste style module that exports flow types for GraphQL enums. // Haste style module that exports flow types for GraphQL enums.
// TODO(T22422153) support non-haste environments // TODO(T22422153) support non-haste environments
enumsHasteModule?: string, enumsHasteModule?: string,
validationRules?: {
GLOBAL_RULES?: $ReadOnlyArray<ValidationRule>,
LOCAL_RULES?: $ReadOnlyArray<ValidationRule>,
},
printModuleDependency?: string => string, printModuleDependency?: string => string,
filesystem?: Filesystem, filesystem?: Filesystem,
repersist?: boolean, repersist?: boolean,
Expand All @@ -84,7 +80,6 @@ function compileAll({
schema, schema,
compilerTransforms, compilerTransforms,
documents, documents,
extraValidationRules,
reporter, reporter,
typeGenerator, typeGenerator,
}: {| }: {|
Expand All @@ -93,27 +88,13 @@ function compileAll({
schema: Schema, schema: Schema,
compilerTransforms: RelayCompilerTransforms, compilerTransforms: RelayCompilerTransforms,
documents: $ReadOnlyArray<DocumentNode>, documents: $ReadOnlyArray<DocumentNode>,
extraValidationRules?: {
GLOBAL_RULES?: $ReadOnlyArray<ValidationRule>,
LOCAL_RULES?: $ReadOnlyArray<ValidationRule>,
},
reporter: Reporter, reporter: Reporter,
typeGenerator: TypeGenerator, typeGenerator: TypeGenerator,
|}) { |}) {
// Verify using local and global rules, can run global verifications here
// because all files are processed together
const validationRules = extraValidationRules
? [
...(extraValidationRules.LOCAL_RULES || []),
...(extraValidationRules.GLOBAL_RULES || []),
]
: [];

const definitions = ASTConvert.convertASTDocumentsWithBase( const definitions = ASTConvert.convertASTDocumentsWithBase(
schema, schema,
baseDocuments, baseDocuments,
documents, documents,
validationRules,
RelayParser.transform, RelayParser.transform,
); );
const compilerContext = new CompilerContext(schema).addAll(definitions); const compilerContext = new CompilerContext(schema).addAll(definitions);
Expand Down Expand Up @@ -172,7 +153,6 @@ function writeAll({
baseDocuments: baseDocuments.valueSeq().toArray(), baseDocuments: baseDocuments.valueSeq().toArray(),
compilerTransforms: writerConfig.compilerTransforms, compilerTransforms: writerConfig.compilerTransforms,
documents: documents.valueSeq().toArray(), documents: documents.valueSeq().toArray(),
extraValidationRules: writerConfig.validationRules,
reporter, reporter,
typeGenerator: writerConfig.typeGenerator, typeGenerator: writerConfig.typeGenerator,
}); });
Expand Down
26 changes: 2 additions & 24 deletions packages/relay-compiler/core/ASTConvert.js
View file
Expand Up @@ -11,7 +11,6 @@
'use strict'; 'use strict';


const Profiler = require('./GraphQLCompilerProfiler'); const Profiler = require('./GraphQLCompilerProfiler');
const RelayValidator = require('./RelayValidator');


const { const {
isExecutableDefinitionAST, isExecutableDefinitionAST,
Expand All @@ -30,7 +29,6 @@ import type {
OperationDefinitionNode, OperationDefinitionNode,
TypeSystemDefinitionNode, TypeSystemDefinitionNode,
TypeSystemExtensionNode, TypeSystemExtensionNode,
ValidationRule,
} from 'graphql'; } from 'graphql';


type ASTDefinitionNode = FragmentDefinitionNode | OperationDefinitionNode; type ASTDefinitionNode = FragmentDefinitionNode | OperationDefinitionNode;
Expand All @@ -42,7 +40,6 @@ type TransformFn = (
function convertASTDocuments( function convertASTDocuments(
schema: Schema, schema: Schema,
documents: $ReadOnlyArray<DocumentNode>, documents: $ReadOnlyArray<DocumentNode>,
validationRules: $ReadOnlyArray<ValidationRule>,
transform: TransformFn, transform: TransformFn,
): $ReadOnlyArray<Fragment | Root> { ): $ReadOnlyArray<Fragment | Root> {
return Profiler.run('ASTConvert.convertASTDocuments', () => { return Profiler.run('ASTConvert.convertASTDocuments', () => {
Expand All @@ -57,20 +54,14 @@ function convertASTDocuments(
}); });
}); });


return convertASTDefinitions( return convertASTDefinitions(schema, definitions, transform);
schema,
definitions,
validationRules,
transform,
);
}); });
} }


function convertASTDocumentsWithBase( function convertASTDocumentsWithBase(
schema: Schema, schema: Schema,
baseDocuments: $ReadOnlyArray<DocumentNode>, baseDocuments: $ReadOnlyArray<DocumentNode>,
documents: $ReadOnlyArray<DocumentNode>, documents: $ReadOnlyArray<DocumentNode>,
validationRules: $ReadOnlyArray<ValidationRule>,
transform: TransformFn, transform: TransformFn,
): $ReadOnlyArray<Fragment | Root> { ): $ReadOnlyArray<Fragment | Root> {
return Profiler.run('ASTConvert.convertASTDocumentsWithBase', () => { return Profiler.run('ASTConvert.convertASTDocumentsWithBase', () => {
Expand Down Expand Up @@ -127,19 +118,13 @@ function convertASTDocumentsWithBase(
requiredDefinitions.forEach(definition => requiredDefinitions.forEach(definition =>
definitionsToConvert.push(definition), definitionsToConvert.push(definition),
); );
return convertASTDefinitions( return convertASTDefinitions(schema, definitionsToConvert, transform);
schema,
definitionsToConvert,
validationRules,
transform,
);
}); });
} }


function convertASTDefinitions( function convertASTDefinitions(
schema: Schema, schema: Schema,
definitions: $ReadOnlyArray<DefinitionNode>, definitions: $ReadOnlyArray<DefinitionNode>,
validationRules: $ReadOnlyArray<ValidationRule>,
transform: TransformFn, transform: TransformFn,
): $ReadOnlyArray<Fragment | Root> { ): $ReadOnlyArray<Fragment | Root> {
const operationDefinitions: Array<ASTDefinitionNode> = []; const operationDefinitions: Array<ASTDefinitionNode> = [];
Expand All @@ -148,13 +133,6 @@ function convertASTDefinitions(
operationDefinitions.push(definition); operationDefinitions.push(definition);
} }
}); });

const validationAST = {
kind: 'Document',
definitions: operationDefinitions,
};
// Will throw an error if there are validation issues
RelayValidator.validate(schema, validationAST, validationRules);
return transform(schema, operationDefinitions); return transform(schema, operationDefinitions);
} }


Expand Down
49 changes: 0 additions & 49 deletions packages/relay-compiler/core/RelayValidator.js
View file

This file was deleted.

13 changes: 0 additions & 13 deletions packages/relay-compiler/core/Schema.js
View file
Expand Up @@ -30,18 +30,15 @@ const {
parse, parse,
parseType, parseType,
print, print,
validate,
} = require('graphql'); } = require('graphql');


import type {Field as GraphQLIRField} from './GraphQLIR'; import type {Field as GraphQLIRField} from './GraphQLIR';
import type { import type {
DirectiveLocationEnum, DirectiveLocationEnum,
DocumentNode, DocumentNode,
GraphQLArgument, GraphQLArgument,
GraphQLError,
Source, Source,
TypeNode, TypeNode,
ValidationRule,
ValueNode, ValueNode,
} from 'graphql'; } from 'graphql';


Expand Down Expand Up @@ -1415,16 +1412,6 @@ class Schema {
return !this.isServerDefinedField(type, field); return !this.isServerDefinedField(type, field);
} }


/**
* This method should be replaced with the specific Relay validations
*/
DEPRECATED__validate(
document: DocumentNode,
rules: $ReadOnlyArray<ValidationRule>,
): $ReadOnlyArray<GraphQLError> {
return validate(this._extendedSchema, document, rules);
}

/** /**
* The only consumer of this is RelayParser.parse(...) * The only consumer of this is RelayParser.parse(...)
* We should either refactor RelayParser.parse(...) to not-rely on the ` * We should either refactor RelayParser.parse(...) to not-rely on the `
Expand Down
34 changes: 0 additions & 34 deletions packages/relay-compiler/core/__tests__/RelayValidator-test.js
View file

This file was deleted.

11 changes: 0 additions & 11 deletions packages/relay-compiler/core/__tests__/__snapshots__/RelayValidator-test.js.snap
View file

This file was deleted.

2 changes: 0 additions & 2 deletions packages/relay-compiler/index.js
View file
Expand Up @@ -35,7 +35,6 @@ const RelayIRValidations = require('./core/RelayIRValidations');
const RelayParser = require('./core/RelayParser'); const RelayParser = require('./core/RelayParser');
const RelaySchema = require('./core/Schema'); const RelaySchema = require('./core/Schema');
const RelaySourceModuleParser = require('./core/RelaySourceModuleParser'); const RelaySourceModuleParser = require('./core/RelaySourceModuleParser');
const RelayValidator = require('./core/RelayValidator');
const Rollout = require('./util/Rollout'); const Rollout = require('./util/Rollout');
const SchemaUtils = require('./core/SchemaUtils'); const SchemaUtils = require('./core/SchemaUtils');


Expand Down Expand Up @@ -143,7 +142,6 @@ module.exports = {


Parser: RelayParser, Parser: RelayParser,
Schema: RelaySchema, Schema: RelaySchema,
Validator: RelayValidator,
CodeGenerator: RelayCodeGenerator, CodeGenerator: RelayCodeGenerator,
FlowGenerator: RelayFlowGenerator, FlowGenerator: RelayFlowGenerator,


Expand Down
1 change: 0 additions & 1 deletion packages/relay-test-utils-internal/parseGraphQLText.js
View file
Expand Up @@ -28,7 +28,6 @@ function parseGraphQLText(
const definitions = convertASTDocuments( const definitions = convertASTDocuments(
Schema.DEPRECATED__create(schema, extendedSchema), Schema.DEPRECATED__create(schema, extendedSchema),
[ast], [ast],
[],
Parser.transform.bind(Parser), Parser.transform.bind(Parser),
); );
return { return {
Expand Down

5 comments on commit 15e8d22

@mrtnzlml
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @alunyov! Is here some replacement for the removed validationRules? We are using it for some extra validation rules. Thanks for your answer.

@alunyov
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, @mrtnzlml we've moved most of the GraphQL validation rules to RelayParser directly.

But some of them became a RelayTransforms - that doesn't change the IR, but validating the correctness of them.

For example (DisallowIdAsAlias): d55420b

The end-goal for us is to remove the dependency on the graphql-js schema implementation (which is used in GraphQL validations).

So, if you have extra validation rules, you can make them Relay Transforms. And configure the compiler to use these transforms.

Just our of curiosity, are those extra validation rules you mentioned will apply to Relay in general?

@mrtnzlml
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! :)

Just our of curiosity, are those extra validation rules you mentioned will apply to Relay in general?

I don't think so. I am experimenting with it. We have basically two validators:

  • validation of deprecated fields, and
  • experimental result size validation

It looks like this:

Screenshot_2019-10-15_at_17 07 46

Deprecated fields could be even Eslint rule but the idea was to show warnings first and after some time (specified in deprecation reason?) we could start throwing an error. It would be more difficult with Eslint. Moreover, we could even automatically transform the fields into the new version.

The second validation is implementation of this idea: http://blog.liu.se/olafhartig/2018/08/08/lightweight-summary-of-our-paper-semantics-and-complexity-of-graphql/ (we will move it to the server, but I need it to figure out what should be the threshold for real-world queries).

So probably not useful at all for Relay community.

@alunyov
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you'll probably need to re-implement those as RelayTransform

For the second, I think, it's even better - because you can run it on already optimized (by compiler) queries, which will give you more accurate results.

@mrtnzlml
Copy link
Contributor

Choose a reason for hiding this comment

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

For the second, I think, it's even better - because you can run it on already optimized (by compiler) queries, which will give you more accurate results.

Yes, definitely! I was trying to figure out how to run it on the transformed queries. Thank you very much! :)

Please sign in to comment.