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

Allow new interface style for GraphQL. #4012

Merged
merged 2 commits into from
Mar 27, 2018

Conversation

ruiaraujo
Copy link
Contributor

This is a breaking change since it will upgrade the old style to the new one.

Closes #3600.

try {
return parse(source, { allowLegacySDLImplementsInterfaces: false });
} catch (_) {
return parse(source, { allowLegacySDLImplementsInterfaces: true });
Copy link
Member

Choose a reason for hiding this comment

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

It'd be great if we could detect which style the code was originally written so we could support both styles, and we can with this code.

But I don't think it's worth the trouble though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree with that.

@suchipi
Copy link
Member

suchipi commented Feb 22, 2018

Will users on older versions of GraphQL libraries be inconvenienced if prettier starts using the new style? As in, will their libraries not be able to parse the new & style? If so, we should probably go through the effort to detect the style from the source.

@ruiaraujo
Copy link
Contributor Author

Yes, I would consider this change a breaking change and consumers should be made aware.

If you disagree and think that we should support it regardless, I would ask for your suggestions on how to do it since the AST provided does not include any notion of separator.

@azz
Copy link
Member

azz commented Feb 22, 2018

We have to check the source (options.originalText)

Rui Araujo added 2 commits March 25, 2018 21:53
This is a breaking change since it will upgrade the old style to the new one.

Closes prettier#3600.
When in present of mixed style, it updates to the new one.
@ruiaraujo
Copy link
Contributor Author

@suchipi @azz I have done it. Feel free to take another look.

Copy link
Member

@suchipi suchipi left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@ruiaraujo
Copy link
Contributor Author

Should I worry about the codecov/project check?

@suchipi
Copy link
Member

suchipi commented Mar 27, 2018

Nah

@suchipi suchipi merged commit 9da8752 into prettier:master Mar 27, 2018
@ruiaraujo ruiaraujo deleted the update_graphql branch March 27, 2018 06:54
@ruiaraujo ruiaraujo mentioned this pull request Mar 31, 2018
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants