Skip to content

definition: replace object spread with explicit assignments #2089

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
Merged
merged 1 commit into from
Aug 14, 2019
Merged

definition: replace object spread with explicit assignments #2089

merged 1 commit into from
Aug 14, 2019

Conversation

IvanGoncharov
Copy link
Member

In preparation for #1527

@IvanGoncharov IvanGoncharov added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Aug 14, 2019
@IvanGoncharov
Copy link
Member Author

Also, it strangely produces significant speedup (almost 50%) for buildSchema and buildClientSchema functions:
image
image

@IvanGoncharov IvanGoncharov force-pushed the master branch 2 times, most recently from 70de26a to e04d92b Compare August 14, 2019 14:35
@IvanGoncharov IvanGoncharov merged commit 0105d92 into graphql:master Aug 14, 2019
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Aug 15, 2019
@IvanGoncharov
definitions: make constructed fields non-optional
df05745 
Reflects changes in graphql#2089
@IvanGoncharov IvanGoncharov mentioned this pull request Aug 15, 2019
Merged
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Aug 15, 2019
@IvanGoncharov
definitions: make constructed fields non-optional
f498b7b 
Reflects changes in graphql#2089
IvanGoncharov added a commit that referenced this pull request Aug 15, 2019
@IvanGoncharov
definitions: make constructed fields non-optional (#2091)
901c4e7 
Reflects changes in #2089
@taion
Copy link
Contributor

taion commented Aug 23, 2019

@IvanGoncharov

This is sort of a breaking change in practice. Because of the older spread-through, it was possible to define arbitrary other properties on fields, e.g. for https://github.com/4Catalyzer/graphql-validation-complexity#usage. This now no longer works.

It's true that this was undocumented behavior, but I also don't think it was quite that rare in practice.

@IvanGoncharov
Copy link
Member Author

@taion Can you please open an issue?
Can you switch to extensions mechanism added in #2097?

but I also don't think it was quite that rare in practice.

If there are devs stuck with this hack and can't switch to extensions I don't have a problem to temporary revert it until 15.0.0.
But it is 100% happening in 15.0.0 and everyone will need to switch to extensions anyway.

@taion taion mentioned this pull request Aug 23, 2019
Closed
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Aug 23, 2019
@IvanGoncharov
Copy non-standard properites from field's config
0c60413 
Fixes graphql#2114
Partly reverts graphql#2089
@IvanGoncharov IvanGoncharov mentioned this pull request Aug 23, 2019
Merged
IvanGoncharov added a commit that referenced this pull request Aug 23, 2019
@IvanGoncharov
Copy non-standard properites from field's config (#2116)
3104e1f 
Fixes #2114
Partly reverts #2089
This was referenced Aug 31, 2019
Merged
Merged
Merged
Merged
Merged
This was referenced Oct 26, 2019
Open
Open
@snyk-bot snyk-bot mentioned this pull request Dec 5, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Jan 16, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Feb 12, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request Jan 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@IvanGoncharov @taion