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.

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

[HOLD] [relay-runtime] Update fragment reference definitions #39231

Merged
merged 1 commit into from Oct 21, 2019
Merged

[HOLD] [relay-runtime] Update fragment reference definitions #39231

merged 1 commit into from Oct 21, 2019

Conversation

alloy
Copy link
Collaborator

@alloy alloy commented Oct 18, 2019 •
edited

⚠️ This is a breaking change, but alas there’s no other way around it.

As of TS 3.6 an intersection of multiple unique symbols results in
never, which means our fragment reference checking is now unsafe.

Depends on relay-tools/relay-compiler-language-typescript#139

@alloy alloy self-assigned this Oct 18, 2019
@alloy
Copy link
Collaborator Author

alloy commented Oct 18, 2019 •
edited

cc @ds300

@alloy alloy force-pushed the update-relay-runtime-for-new-fragment-refs branch 3 times, most recently from 39493bb to 4264f77 Compare October 18, 2019 10:54
@alloy
Copy link
Collaborator Author

alloy commented Oct 18, 2019 •
edited

So we tried to add package.json file that listed the latest version of the relay-compiler-language-typescript plugin as a peer dep, so people updating just the DT typings would get a warning about unmatched versions. Alas that doesn’t appear to be possible:

Error: /home/travis/build/DefinitelyTyped/DefinitelyTyped/types/relay-runtime/package.json should not include field peerDependencies

See also #20290

@alloy alloy mentioned this pull request Oct 18, 2019
Closed
1 task
@alloy alloy force-pushed the update-relay-runtime-for-new-fragment-refs branch from 4264f77 to bfc99d0 Compare October 18, 2019 11:04
@alloy
[relay-runtime] Update fragment reference definitions
903bf1a 
As of TS 3.6 an intersection of multiple unique symbols results in
`never`, which means our fragment reference checking is now unsafe.
@alloy alloy force-pushed the update-relay-runtime-for-new-fragment-refs branch from bfc99d0 to 903bf1a Compare October 18, 2019 11:10
@typescript-bot typescript-bot added this to Check and Merge in Pull Request Status Board Oct 18, 2019
@typescript-bot typescript-bot added Awaiting reviewer feedback Author is Owner The author of this PR is a listed owner of the package. labels Oct 18, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Oct 18, 2019 •
edited

@alloy Thank you for submitting this PR!

🔔 @Graphcool @voxmatt @npirotte @ckknight @kastermester @mattkrick @renanmav - 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.

@typescript-bot
Copy link
Contributor

typescript-bot commented Oct 18, 2019 •
edited

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@alloy alloy changed the title [HOLD] [relay-runtime] Update fragment reference definitions [relay-runtime] Update fragment reference definitions Oct 18, 2019
@kastermester
Copy link

I'm a bit pressured for time right now, but having taken a quick glance at this I think it looks fine. :)

@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

react-relay/v6

Comparison details for react-relay/v6 📊
master #39231 diff
Batch compilation
Memory usage (MiB) 139.8 139.2 -0.4%
Type count 25469 25476 0%
Assignability cache size 54554 54557 0%
Language service
Samples taken 742 730 -2%
Identifiers in tests 742 730 -2%
getCompletionsAtPosition
    Mean duration (ms) 699.3 728.1 +4.1%
    Mean CV 11.9% 9.8%
    Worst duration (ms) 884.0 924.1 +4.5%
    Worst identifier div ClassComponent3
getQuickInfoAtPosition
    Mean duration (ms) 659.1 678.0 +2.9%
    Mean CV 12.8% 10.6% -17.5%
    Worst duration (ms) 852.6 844.6 -0.9%
    Worst identifier relay query

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

relay-runtime/v6

Comparison details for relay-runtime/v6 📊
master #39231 diff
Batch compilation
Memory usage (MiB) 85.3 78.9 -7.4%
Type count 11061 11066 0%
Assignability cache size 34852 34855 0%
Language service
Samples taken 128 128 0%
Identifiers in tests 128 128 0%
getCompletionsAtPosition
    Mean duration (ms) 444.0 439.1 -1.1%
    Mean CV 12.3% 12.8%
    Worst duration (ms) 566.6 528.4 -6.7%
    Worst identifier ConcreteRequest ROOT_ID
getQuickInfoAtPosition
    Mean duration (ms) 412.3 416.2 +1.0%
    Mean CV 10.9% 11.5% +6.1%
    Worst duration (ms) 547.6 566.9 +3.5%
    Worst identifier setValue setValue

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Oct 18, 2019
@alloy
Copy link
Collaborator Author

alloy commented Oct 18, 2019

@kastermester Thanks! I’ll go forward and self-merge now then, as we need to sync the release with the plugin.

@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. and removed Awaiting reviewer feedback labels Oct 18, 2019
@alloy
Copy link
Collaborator Author

alloy commented Oct 18, 2019

We’re holding-off merging until we know the https://github.com/relay-tools/relay-compiler-language-typescript release process is in a good place. /cc @zephraph

@alloy alloy changed the title [relay-runtime] Update fragment reference definitions [HOLD] [relay-runtime] Update fragment reference definitions Oct 18, 2019
@alloy
Copy link
Collaborator Author

alloy commented Oct 21, 2019

Alright, the new plugin version is released, so going to merge this now too.

@alloy alloy merged commit b5210ed into master Oct 21, 2019
Pull Request Status Board automation moved this from Check and Merge to Done Oct 21, 2019
@alloy alloy deleted the update-relay-runtime-for-new-fragment-refs branch October 21, 2019 16:46
@typescript-bot
Copy link
Contributor

I just published @types/relay-runtime@6.0.7 to npm.

@renanmav
Copy link
Contributor

Which plugin @alloy?

@alloy
Copy link
Collaborator Author

alloy commented Oct 21, 2019

The TS plugin for relay-compiler relay-tools/relay-compiler-language-typescript#139

chivesrs pushed a commit to chivesrs/DefinitelyTyped that referenced this pull request Nov 19, 2019
@alloy @chivesrs
[relay-runtime] Update fragment reference definitions (DefinitelyType…
1f49f11 
…d#39231)

As of TS 3.6 an intersection of multiple unique symbols results in
`never`, which means our fragment reference checking is now unsafe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@renanmav renanmav renanmav approved these changes

Assignees

@alloy alloy

Labels
Author is Owner The author of this PR is a listed owner of the package. Owner Approved A listed owner of this package signed off on the pull request. Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alloy @typescript-bot @kastermester @renanmav