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

@types/graphql - provide type definitions for mergeAST utility #35942

Closed
wants to merge 1 commit into from

Conversation

acao
Copy link

@acao acao commented Jun 4, 2019

For a simple new utility we are introducing: graphql/graphql-js#1948

Needed because we are converting GraphiQL to typescript, and moving this utility from GraphiQL to graphql-js.

We should wait until the aforementioned PR is merged, of course

@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

**Comparison details** 📊
master #35942 diff
Batch compilation
Type count 3181 3181 0%
Assignability cache size 169 169 0%
Subtype cache size 0 0
Identity cache size 0 0
Language service
Samples taken 230 230 0%
Identifiers in tests 46 46 0%
getCompletionsAtPosition
    Mean duration (ms) 185.9 185.0 -0.5%
    Median duration (ms) 172.1 171.5 -0.4%
    Std. deviation (ms) 45.7 46.6 +1.9%
    Worst duration (ms) 222.9 222.1 -0.4%
    Worst identifier utilities_index_tests utilities_typeFromAST_tests
getQuickInfoAtPosition
    Mean duration (ms) 183.2 179.3 -2.1%
    Median duration (ms) 168.9 163.1 -3.5%
    Std. deviation (ms) 44.1 41.7 -5.6%
    Worst duration (ms) 226.6 216.8 -4.3%
    Worst identifier utilities_TypeInfo_tests utilities_astFromValue_tests

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.


If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

**Comparison details** 📊
master #35942 diff
Batch compilation
Type count 3181 3181 0%
Assignability cache size 169 169 0%
Subtype cache size 0 0
Identity cache size 0 0
Language service
Samples taken 230 230 0%
Identifiers in tests 46 46 0%
getCompletionsAtPosition
    Mean duration (ms) 185.9 182.6 -1.8%
    Median duration (ms) 172.1 166.4 -3.3%
    Std. deviation (ms) 45.7 48.4 +5.9%
    Worst duration (ms) 222.9 217.6 -2.4%
    Worst identifier utilities_index_tests utilities_buildASTSchema_tests
getQuickInfoAtPosition
    Mean duration (ms) 183.2 181.4 -1.0%
    Median duration (ms) 168.9 165.8 -1.8%
    Std. deviation (ms) 44.1 45.3 +2.6%
    Worst duration (ms) 226.6 228.4 +0.8%
    Worst identifier utilities_TypeInfo_tests error_formatError
System information
Node version v10.15.3 v10.15.3
CPU count 2 2
CPU speed 2.294 GHz 2.397 GHz
CPU model Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz
CPU Architecture x64 x64
Memory 6.8 GiB 6.8 GiB
Platform linux linux
Release 4.15.0-1041-azure 4.15.0-1041-azure

First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt.

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.


If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

@typescript-bot typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Jun 4, 2019
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Jun 4, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 4, 2019

@acao Thank you for submitting this PR!

🔔 @TonyPythoneer @calebmer @intellix @firede @kepennar @freiksenet @IvanGoncharov @DxCx @rportugal @tgriesser @dyst5422 @adnsio @divyenduz @bradzacher @clayne11 @JCMais @langpavel @mc0 @martijnwalraven - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

Copy link
Contributor

@langpavel langpavel left a comment

Choose a reason for hiding this comment

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

Not merged yet and not released. Wait for graphql/graphql-js#1948

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Jun 4, 2019
@typescript-bot typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback labels Jun 4, 2019
@typescript-bot
Copy link
Contributor

@acao One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

@acao
Copy link
Author

acao commented Jun 4, 2019

@langpavel indeed, i mentioned that in the PR description, but I think I'll just close it for now anyways

@acao acao closed this Jun 4, 2019
Pull Request Status Board automation moved this from Needs Author Attention to Done Jun 4, 2019
@langpavel
Copy link
Contributor

Well, @acao, I hope you understand. Unfortunately DefinitelyTyped is managed by bot at some point, so I must reject this PR or suggest changes.
It may be unfortunate especially for small libraries because bot will accept changes if someone approve them, there is only trust.
I must admit that I'm interested in feature — so I looked at; heh 😃 there are so many variants of this. At least 5 packages have similar utility function, but each behave differently....

@acao
Copy link
Author

acao commented Jun 5, 2019

@langpavel of course, please do what you need. feel free to reject it.

yeah it's no small effort! this originally began as an effort to move the utility from graphiql to graphql-js, but it turns out theres a lot of thought that would go into it. currently this utility is used in graphiql for a "merge queries" button that we now realize is quite precarious, and could lead to losing significant parts of your query in complex situations. back to the drawing board, I'll need to approach this in a very careful manner when re-opening it so i dont waste your and Ivan's time again, haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Popular package This PR affects a popular package (as counted by NPM download counts). Revision needed This PR needs code changes before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants