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

Bugfix : gatsby-source-wordpress giving invalid node names to GraphQL #1778

Merged
merged 5 commits into from
Aug 16, 2017

Conversation

sebastienfi
Copy link
Contributor

Should solve #1774

  • Modified regex validating node names.
  • Corrected some issue with the node referencing itself introduced in latest commit.
  • Also prefixed almost every console.log() to bring a quieter terminal at build, keeping only necessary info (we still need to see that the process did not hang on heavy sites).

@jquense
We surely need a better validation of names from GraphQL because /^[_a-zA-Z][_a-zA-Z0-9]*$/ see as valid some names that will break later on : graphql/graphql-js#632
Example :
building schemaName "__jetpack_v4" must not begin with "__", which is reserved by GraphQL introspection. In a future release of graphql this will become a hard error.

Modified regex validating node names.
#1774

This could be in cause, we surely need a better validation of names from GraphQL because `/^[_a-zA-Z][_a-zA-Z0-9]*$/` see as valid some names that will break later on : graphql/graphql-js#632

`building schemaName "__jetpack_v4" must not begin with "__", which is reserved by GraphQL introspection. In a future release of graphql this will become a hard error.`
@sebastienfi sebastienfi changed the title Bugfix : gatsby-source-wordpress Bugfix : gatsby-source-wordpress giving invalid node names to GraphQL Aug 11, 2017
@gatsbybot
Copy link
Collaborator

gatsbybot commented Aug 11, 2017

Deploy preview ready!

Built with commit 98dbdea

https://deploy-preview-1778--using-drupal.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Aug 11, 2017

Deploy preview ready!

Built with commit 98dbdea

https://deploy-preview-1778--gatsbygram.netlify.com

colorized.out(
`=START PLUGIN=====================================`,
colorized.color.Font.FgBlue
if (_verbose) console.log()
Copy link
Contributor

Choose a reason for hiding this comment

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

plugins are now passed a reporter object for doing this, if you want to make use of it. e.g. reporter.verbose('blah')

Copy link
Contributor

Choose a reason for hiding this comment

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

@jquense it'd be cool to auto-prefix the plugin name as well to messages

if (!NAME_RX.test(nkey)) {
nkey = `_${nkey}`.replace(/-/g, `_`).replace(/:/g, `_`)
if (!NAME_RX.test(nkey) || restrictedNodeFields.includes(nkey)) {
nkey = `${conflictFieldPrefix}${nkey}`.replace(/-|__|:|\.|\s/g, `_`)
Copy link
Contributor

@jquense jquense Aug 11, 2017

Choose a reason for hiding this comment

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

so we should provide a single robust utility for doing this (there is one in gatsby), but this is still incomplete in terms of what graphql will allow. It needs to replace any character that isn't alphanumeric or a _. It also can't start with a number or two or more _. e.g here are some fields that would still error out here.

  • "##foo"
  • "1number"

@corysimmons
Copy link
Contributor

Is this the problem I'm experiencing in #1321 (comment) ?

@sebastienfi sebastienfi merged commit d03f296 into master Aug 16, 2017
@sebastienfi sebastienfi deleted the sebastienfi-patch-1 branch August 16, 2017 08:07
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.

5 participants