[fix] experimental compiler: allow default args on required fields #3612
Conversation
|
Hi @tbezman! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
4 similar comments
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@captbaritone has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Awesome! Glad to see that you've managed to get the compiler building well enough to start hitting issues. This matches with my read of the spec here: https://spec.graphql.org/June2018/#sec-Required-Arguments I've imported this PR into our system to get it merged. Great to see you already contributing, thanks! |
|
It looks like there are some superficial test fixtures changes that need to be updated. Adding a new field to the test schema slightly changes the serialized value in some test output. Fixing this should be as simple as running: cd oss/crates/graphql-ir
UPDATE_SNAPSHOTS=1 cargo test |
|
@captbaritone Oh nice, I manually created that snapshot file haha. Let me know if things are looking better in the latest commit. |
|
@captbaritone has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Also, @captbaritone are y'all planning on publishing the latest version of the experimental compiler soon or should we just be building from source until it becomes more stable? |
|
It looks like there is currently some internal dependence on this behavior, despite the fact that required arguments with defaults does seem to be valid as per the spec. I think the ideal solution would be to make this change (so that our IR conforms to the spec) and move this check into a separate validation pass that's Facebook only, or is at least configurable. I'm going to talk to some folks internally to better understand what's at play, and get back to you. As for our release, I don't think we're planning on publishing new releases of the compiler until we do an official release later this |
|
@captbaritone That seems like a reasonable solution to me. And regarding the release, if y'all are rolling out later this month then it shouldn't be an issue. I don't think we have a lot of Rust tooling at Coinbase and would not want to be the first to set it up haha |
|
Oops. Typo earlier. I said "this month" when I meant to write "this half". |
|
@captbaritone Still seems like a reasonable timeline for us. Looking forward to it. |
|
Sounds good. I still need to talk with some folks about this specific PR.
Just wanted to let you known I’m still tracking this.
On Tue, Oct 19, 2021 at 10:25 AM Terence Bezman ***@***.***> wrote:
@captbaritone <https://github.com/captbaritone> Still seems like a
reasonable timeline for us. Looking forward to it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3612 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABHXL52HP63YJSAVNMYH53UHWSYPANCNFSM5FUXRPRQ>
.
--
Jordan Eldredge
jordaneldredge.com
@captbaritone <http://twitter.com/captbaritone>
|
|
@captbaritone Any updates on this front? I know you guys have a list of things to do but I'm curious of timeline stuff here. Coinbase is trying to do this upgrade in Q1 2022 |
|
Hey! Thanks for pinging, and sorry for the delay. We'll be meeting internally tomorrow to continue this discussion and hopefully have a good answer then. Either way, I'll provide an updater here tomorrow afternoon. |
|
@captbaritone has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Merged. Thanks @tbezman! |
At Coinbase, we were trying out the experimental compiler and this was the first issue we ran into.
The JS compiler allowed this and it seems valid. Let me know if this is out of spec.
CC @kassens @captbaritone