Skip to content

Do not warn about legacy __configs__ name #772

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

Closed
wants to merge 1 commit into from

Conversation

kassens
Copy link
Contributor

@kassens kassens commented Mar 29, 2017

We currently don't have a good way to silence warnings for legacy field names staring with __. This silences the two offenders used internally at Facebook until we find a way to clean them up from the schema or ignore them when parsing an existing schema.

@wincent
Copy link
Contributor

wincent commented Mar 29, 2017

Rather than put an FB-specific hack in the reference implementation, I'd much rather see us come up with a mechanism you could use to add exceptions to a whitelist. (Or failing that, a way of opting out of this validation.) It wouldn't need to be pretty, ergonomic or easy (we don't want to make opting out too easy), just possible. What do you think?

@kassens
Copy link
Contributor Author

kassens commented Mar 29, 2017

Thoughts on how to do this? I'd like to avoid passing a flag through the call stack, which I'd argue pollutes this implementation even more then a small special case.

I could add a flag on assertValidName? Something like require('graphql').assertValidName.WARN = false, but that relies on calling that before loading the schema and mutating a module.

@leebyron
Copy link
Contributor

IMHO we should create warnings if you're creating a schema manually for runtime, but not create warnings if you're hydrating a schema from an introspection result. It's pretty useless for tools like graphiql or codegen systems to issue these warnings that are really in place for first-time schema construction.

Threading a variable through everything feels ugly, but so setting a global value :/

@leebyron
Copy link
Contributor

For example, maybe in buildClientSchema or buildASTSchema we can set some global context flag to ignore warnings?

@leebyron
Copy link
Contributor

I think most ideally validating a schema and constructing a schema can be separated into two separate functions. That would also improve schema construction performance by not running this validation inline every time. Perhaps schema validation could them be run as part of the all-inclusive graphql function.

@wincent
Copy link
Contributor

wincent commented Mar 29, 2017

For reference, #719 is the issue tracking the separation of warnings according to runtime construction vs introspection.

@kassens kassens closed this Apr 4, 2017
@kassens kassens deleted the warn branch April 4, 2017 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants