-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add relay-config library #4780
base: main
Are you sure you want to change the base?
Add relay-config library #4780
Conversation
Would love a more complete understanding of what problem(s) we're trying to solve here. Here's what I'm inferring: The babel plugin currently needs to be able to see the Relay config file in order to apply the correct transformation. This requires:
Am I correct in inferring that this is the set of problems we are trying to solve here? |
package.json
Outdated
@@ -37,6 +37,7 @@ | |||
"@babel/traverse": "^7.14.0", | |||
"@babel/types": "^7.0.0", | |||
"@jest/create-cache-key-function": "^29.7.0", | |||
"@rushstack/node-core-library": "^5.7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant to bring in this dependency for a set of things that look like pretty thin wrappers around node apis. Do you think it could be avoided? What's the cost of not including it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using it for validating the loaded config against the json schema so we would otherwise need some dependency for doing that (e.g. ajv
which this library uses under the hood). also this dep should only be used in dev tools not anything at runtime
package.json
Outdated
@@ -47,6 +48,8 @@ | |||
"del": "6.0.0", | |||
"eslint": "^8.57.0", | |||
"eslint-config-fbjs": "4.0.0", | |||
"mock-fs": "^5.2.0", | |||
"json-schema-to-typescript": "^13.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually use this to generate typescript types, or just for json-schema runtime validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using in a gulp task in the build to automatically generate from the schema file so we don't need to manually keep the two in sync
@captbaritone yeah that's about right. For context I'm working on a nice integration layer right now between Relay and Next.js, but the same idea would apply to any tools building on top of Relay. The two places I'm using
Providing a type-safe/standard library for loading Relay's config file will avoid bugs/hacks in tools building on top of Relay, e.g. see this cli that just hardcodes I will also port the Babel plugin to use this library in a follow-up! |
@@ -83,6 +83,7 @@ function testPackageDependencies(topLevelPackagePath, packagePath) { | |||
'relay-compiler', | |||
'relay-runtime', | |||
'react-relay', | |||
'relay-config', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this should be in here
Adding (or rather reviving) a
relay-config
library for loading/saving Relay configs. Will portbabel-plugin-relay
to use this but hitting "module not found" errors in tests.