Skip to content
This repository has been archived by the owner on Sep 27, 2023. It is now read-only.

Update fixtures and tests with relay-compiler v10.0.0 #196

Merged
merged 7 commits into from
Jul 28, 2020

Conversation

wadamek65
Copy link
Contributor

Even though the tests with new directives pass just fine, actually using the new directives with these changes still does not work.

Submitting draft as I'm not sure what to check next.

Should fix #195 after it is done properly.

@sibelius

@@ -1084,7 +1084,7 @@ function getDataTypeName(name: string): string {
}

// Should match FLOW_TRANSFORMS array
// https://github.com/facebook/relay/blob/v6.0.0/packages/relay-compiler/language/javascript/RelayFlowGenerator.js#L621-L627
// https://github.com/facebook/relay/blob/v10.0.0/packages/relay-compiler/language/javascript/RelayFlowGenerator.js#L982
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only updated the url (or maybe shouldn't have?). The transforms array seems to be in sync with relay-compiler.

@sibelius
Copy link
Contributor

do you have a custom handler?

@wadamek65
Copy link
Contributor Author

By handler do you mean handlerProvider? Then no.

@wadamek65 wadamek65 changed the title Update fixtures and tests with relay-compiler v10.0.0 (#195) Update fixtures and tests with relay-compiler v10.0.0 Jul 16, 2020
@sibelius
Copy link
Contributor

make sure the handler is being called

https://github.com/facebook/relay/blob/master/packages/relay-runtime/handlers/RelayDefaultHandlerProvider.js#L28

and that generated have the metadata

@wadamek65
Copy link
Contributor Author

By handler being called you mean my own repo from my issue or in relay-compiler-language-typescript ? If you mean my repo I don't think it has anything to do with the error, as I get it during relay compilation, not runtime. I can't even generate relay artifacts. Added the default handler explicitly just to test and I still get the error:

import {
  RelayNetworkLayer,
  uploadMiddleware,
} from 'react-relay-network-modern';
import {DefaultHandlerProvider, Environment, RecordSource, Store } from 'relay-runtime';

const network = new RelayNetworkLayer([uploadMiddleware()]);
const store = new Store(new RecordSource());

export const environment = new Environment({ handlerProvider: DefaultHandlerProvider, network, store });

@wadamek65
Copy link
Contributor Author

Okay, I've figured it out.

I had multiple packages installed that depended on graphql-tools which in turn installed relay-optimizer and it uses relay-compiler v9. yarn insisted on using that binary instead of the v10.0.0 one which resulted in that error. After symlinking it to a correct binary it now works. Still no code completion for the directives but at least the compilation step works properly.

If the PR is of any use I'd be happy to finish it.

@sibelius
Copy link
Contributor

it is working before this changes?

@wadamek65
Copy link
Contributor Author

Yes it does. This package is unrelated to my problem.

@thicodes
Copy link
Contributor

thicodes commented Jul 16, 2020

nice! it is working with the examples?

@wadamek65
Copy link
Contributor Author

Seems like it is working just fine (apart from the code completion). Can't confirm 100% as I'm not quite sure how to go about translating a previous config like this:

          {
            type: 'RANGE_ADD',
            parentID: userData.id,
            edgeName: 'result',
            connectionInfo: [
              { key: 'Normal_profiles', rangeBehavior: 'append' },
            ],
          },

Into the new appendEdge. But it seems to be working all fine as I'm getting an error index.js:1 Warning: [Relay][Mutation] The connection with id 'client:root:viewer:me:QWNjb3VudC41ZjBjYjdmZDRkZjViYTEyMzRhODEyMDc=:Normal_profiles' doesn't exist. So once I actually provide it with a correct ID it should start working.

One strange thing I noticed is that @deleteRecord directive seems to require ID type and does not accept ID! type, even though the RANGE_DELETE config works just fine (unless I am misinformed). Getting error from compiler:

ERROR:
Invalid use of @deleteRecord on field 'id'. Expected field type ID, got ID!.

Source: views/Main/Admin/Accounts/components/DeleteDialog.tsx (8:14)
7:         result {
8:           id @deleteRecord
                ^
9:         }

But this is obviously unrelated to this package.

As I've said before, if this PR is of any use I'd be happy to finish it.

@sibelius
Copy link
Contributor

I think it is useful to finish this

to keep everything up to date

the last part would be to run this changes on relay-examples, also testing it new directives works well there

@wadamek65
Copy link
Contributor Author

Okay, I will work on that next at weekend :) Thank you for your help so far, I love relay and will be happy to contribute.

@wadamek65 wadamek65 marked this pull request as ready for review July 25, 2020 14:43
@wadamek65
Copy link
Contributor Author

@sibelius @thicodes I think this is ready for review. Updated the examples and also extended them with a checkbox "Append" to showcase/test both appendEdge and prependEdge. deleteRecord unfortunately doesn't fit in this example so didn't add it. The new directives are working just fine.

@maraisr
Copy link
Member

maraisr commented Jul 27, 2020

not sure if we push #199 before this or not, both seem to upgrade graphql to v15.

I think its is a more verbose PR, perhaps we can merge these two into each other? cc @alloy @sibelius

@alloy
Copy link
Member

alloy commented Jul 27, 2020

not sure if we push #199 before this or not, both seem to upgrade graphql to v15.

I think its is a more verbose PR, perhaps we can merge these two into each other? cc @alloy @sibelius

Sounds good to me 👍

@thicodes
Copy link
Contributor

thicodes commented Jul 27, 2020

@wadamek65 I created a PR only with the updates of the packages and fixtures and i merged it. You are making great updates to the examples. We really appreciate what you've been doing. Please get the updates and I think we'll be ready to merge

# Conflicts:
#	example-hooks/package.json
#	example-hooks/yarn.lock
#	example/package.json
#	package.json
#	test/__snapshots__/TypeScriptGenerator-test.ts.snap
#	test/fixtures/type-generator/mutation-with-delete-record.graphql
#	yarn.lock
@wadamek65
Copy link
Contributor Author

I think everything should be up to date now @thicodes

@thicodes
Copy link
Contributor

good job!

@thicodes thicodes self-requested a review July 27, 2020 16:14
@alloy
Copy link
Member

alloy commented Jul 28, 2020

Great job @wadamek65, thanks! 🙏

@alloy alloy added the Skip Release Preserve the current version when merged label Jul 28, 2020
@alloy alloy merged commit 4134988 into relay-tools:master Jul 28, 2020
@alloy
Copy link
Member

alloy commented Aug 4, 2020

🚀 PR was released in v13.0.1 🚀

@alloy alloy added the released label Aug 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
released Skip Release Preserve the current version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relay 10: Error when compiling new directives
5 participants