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

Add support for devOnlyAssignments #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

janicduplessis
Copy link

Closes #93

Add support for https://github.com/facebook/relay/blob/f8585ab4f90f2e707d5555c9f7b423de3ea553bd/packages/relay-compiler/language/RelayLanguagePluginInterface.js#L176.

My main questions is whether this is a breaking change or not and if it should be enabled by default. Also should we make the dev check process.env.NODE_ENV !== 'production' configurable?

@alloy
Copy link
Member

alloy commented Mar 12, 2019

Oh! I didn’t even know of this. Great stuff, I guess we should also update the documentation upstream on the interface then.

Ok, so what’s interesting is that upstream appears to always set it when queries are being persisted, so perhaps they strip it out on builds? [Scratch that, as stated below I was confused about what file I was looking at, it actually looks like the OSS version of formatGeneratedModule does nothing with the passed in devOnlyAssignments, so I guess FB has an internal fork?] That would actually be more suitable to our [Artsy’s] setup, as we check our artefacts in and having different artefacts for dev vs prod would lead to a dirty SCM checkout.

I’m not sure if a Babel transform already exists that would strip property initializers from objects–it would certainly not be hard to create–but I do think that perhaps that could lead to a better situation where the person in charge of builds can configure dev vs prod differences in a single place (babel config, in this case).

What do you think?

"(node/*: any*/)",
"(node as any)"
)}\n}`
: "";
Copy link
Member

@alloy alloy Mar 12, 2019

Choose a reason for hiding this comment

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

I think we should just mimic the upstream behaviour, which is to always do this when queries are being persisted https://github.com/facebook/relay/blob/42033ebcc298af7e8ed90632b70077b7545a282d/packages/relay-compiler/codegen/writeRelayGeneratedFile.js#L107-L133

typeText: 'export type CompleteExample = { readonly id: string }',
hash: 'abcde',
sourceHash: 'edcba',
devOnlyAssignments: '(node/*: any*/).params.text = "query CompleteExampleQuery { id }";',
Copy link
Member

Choose a reason for hiding this comment

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

As per the upstream behaviour, the actual query text should be taken from the generated AST node: https://github.com/facebook/relay/blob/42033ebcc298af7e8ed90632b70077b7545a282d/packages/relay-compiler/codegen/writeRelayGeneratedFile.js#L110

Copy link
Member

Choose a reason for hiding this comment

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

Scratch this, I wasn’t paying attention to the filename, as per my below comment.

@alloy
Copy link
Member

alloy commented Mar 12, 2019

Ah, hmm, I now see that the main difference is that upstream is doing this work in writeRelayGeneratedFile, instead of formatGeneratedModule 🤔

I wonder if we could move the Flow type annotation behaviour upstream to formatGeneratedModule instead, because writeRelayGeneratedFile shouldn’t really know about Flow specifically. /cc @jstejada

For now, though, I guess your string replace is the best we can do.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants