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

Support babel-plugin-macros #2171

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@apalm
Contributor

apalm commented Oct 27, 2017

This PR adds support for babel-plugin-macros.

Usage:

import {graphql} from 'react-relay/macro';
// Or: 
// const {graphql} = require('react-relay/macro');

graphql is exported from both the react-relay and relay-runtime packages, but as I created this PR mostly in the interest of possible Create React App integration, I only added an export to react-relay.

This PR currently only adds support for Relay Modern. I'm not sure if Compat and/or Classic should be supported?

I currently only have a single basic test. I certainly could copy over the fixtures in __tests__/fixtures-modern and change the imports, although I'm not sure how beneficial that would be, as most of the logic is shared with babel-plugin-relay.

Closes #1968

Related: facebook/create-react-app#2730 (comment)

/cc @kentcdodds

@kentcdodds

This is looking great! I left a few comments.

Thanks so much for putting the work into it!

@kassens

This is really cool, thanks for working on it!

Seems like it might actually be ready and no longer WIP?

@apalm

This comment has been minimized.

Show comment
Hide comment
@apalm

apalm Oct 27, 2017

Contributor

@kassens Thanks for looking!

Since it sounds like only Modern needs to be supported, I'll update BabelPluginRelay.md to note that this only works in Modern.

I presume your comment is referring to the haste option, and it doesn't need to be supported?

Did you have any opinions on testing? i.e. like using the fixtures in __tests__/fixtures-modern. Also, the current way of testing by importing from ../../react-relay/modern/ReactRelayGraphQL.macro feels iffy, since consumers would import from react-relay/macro, but not sure if there's a better way.

Contributor

apalm commented Oct 27, 2017

@kassens Thanks for looking!

Since it sounds like only Modern needs to be supported, I'll update BabelPluginRelay.md to note that this only works in Modern.

I presume your comment is referring to the haste option, and it doesn't need to be supported?

Did you have any opinions on testing? i.e. like using the fixtures in __tests__/fixtures-modern. Also, the current way of testing by importing from ../../react-relay/modern/ReactRelayGraphQL.macro feels iffy, since consumers would import from react-relay/macro, but not sure if there's a better way.

@DavidBabel

This comment has been minimized.

Show comment
Hide comment
@DavidBabel

DavidBabel Nov 13, 2017

Any status on this PR ?

DavidBabel commented Nov 13, 2017

Any status on this PR ?

@apalm

This comment has been minimized.

Show comment
Hide comment
@apalm

apalm Nov 13, 2017

Contributor

Sorry for dropping the ball; been busy. I plan to update the PR either later this week or early next week.

Contributor

apalm commented Nov 13, 2017

Sorry for dropping the ball; been busy. I plan to update the PR either later this week or early next week.

Show outdated Hide outdated package.json Outdated
@kassens

This comment has been minimized.

Show comment
Hide comment
@kassens

kassens Dec 23, 2017

Member

@apalm: I still think this would be awesome, do you want to continue this or have someone take over?

Member

kassens commented Dec 23, 2017

@apalm: I still think this would be awesome, do you want to continue this or have someone take over?

@apalm

This comment has been minimized.

Show comment
Hide comment
@apalm

apalm Dec 26, 2017

Contributor

@kassens: Yes, I still want to continue this. Sorry again for dropping the ball.

With the latest commits, I:

  • Fixed merge conflicts
  • Changed from babel-macros -> babel-plugin-macros
  • Addressed a few review comments. I followed Kent's suggestion and used babel-plugin-tester, but if you're not keen on the extra dependency, it's easy to revert, as the change is in a separate commit.

Let me know if you need anything else from me.

Contributor

apalm commented Dec 26, 2017

@kassens: Yes, I still want to continue this. Sorry again for dropping the ball.

With the latest commits, I:

  • Fixed merge conflicts
  • Changed from babel-macros -> babel-plugin-macros
  • Addressed a few review comments. I followed Kent's suggestion and used babel-plugin-tester, but if you're not keen on the extra dependency, it's easy to revert, as the change is in a separate commit.

Let me know if you need anything else from me.

@wtgtybhertgeghgtwtg

This comment has been minimized.

Show comment
Hide comment
@wtgtybhertgeghgtwtg

wtgtybhertgeghgtwtg Dec 28, 2017

Is it still a work in progress, then?

wtgtybhertgeghgtwtg commented Dec 28, 2017

Is it still a work in progress, then?

@wtgtybhertgeghgtwtg

This comment has been minimized.

Show comment
Hide comment
@wtgtybhertgeghgtwtg

wtgtybhertgeghgtwtg Jan 18, 2018

babel-plugin-macros support has landed in latest alpha for create-react-app: facebook/create-react-app#3675 (comment)

wtgtybhertgeghgtwtg commented Jan 18, 2018

babel-plugin-macros support has landed in latest alpha for create-react-app: facebook/create-react-app#3675 (comment)

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jan 28, 2018

Collaborator

@apalm Your last comment makes it sound like this PR is no longer a WIP, is that the case? If so, please update the PR title.

Collaborator

alloy commented Jan 28, 2018

@apalm Your last comment makes it sound like this PR is no longer a WIP, is that the case? If so, please update the PR title.

@apalm apalm changed the title from [WIP] Support babel-macros to Support babel-plugin-macros Jan 28, 2018

@apalm

This comment has been minimized.

Show comment
Hide comment
@apalm

apalm Jan 28, 2018

Contributor

@alloy Yes, that's correct. I've updated the title.

Contributor

apalm commented Jan 28, 2018

@alloy Yes, that's correct. I've updated the title.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jan 28, 2018

Collaborator

@kassens @jstejada Good to go?

Collaborator

alloy commented Jan 28, 2018

@kassens @jstejada Good to go?

@facebook-github-bot

@jstejada has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@zachasme

This comment has been minimized.

Show comment
Hide comment
@zachasme

zachasme Jun 4, 2018

This is the last remaining blocker for using relay in CRA@next without ejecting.

zachasme commented Jun 4, 2018

This is the last remaining blocker for using relay in CRA@next without ejecting.

@kassens

This comment has been minimized.

Show comment
Hide comment
@kassens

kassens Aug 8, 2018

Member

Finally found some time to figure out what was wrong in the gulpfile and got a working CRA with Relay 🎉

I couldn't figure out how to push to this branch, so put up my commit with the fixes at kassens/relay@d7321f9

The last things I'm not clear about:

First, I want to avoid a dependency from relay-runtime on the babel plugin, so I moved the macro into the babel plugin:

import graphql from "babel-plugin-relay/macro";

Is this weird? We could also move to a new package, maybe import graphql from 'relay.macro'?

Second, should babel-plugin-relay have some sort of dependency on babel-plugin-macros as there's a require in there?

Member

kassens commented Aug 8, 2018

Finally found some time to figure out what was wrong in the gulpfile and got a working CRA with Relay 🎉

I couldn't figure out how to push to this branch, so put up my commit with the fixes at kassens/relay@d7321f9

The last things I'm not clear about:

First, I want to avoid a dependency from relay-runtime on the babel plugin, so I moved the macro into the babel plugin:

import graphql from "babel-plugin-relay/macro";

Is this weird? We could also move to a new package, maybe import graphql from 'relay.macro'?

Second, should babel-plugin-relay have some sort of dependency on babel-plugin-macros as there's a require in there?

@kentcdodds

This comment has been minimized.

Show comment
Hide comment
@kentcdodds

kentcdodds Aug 8, 2018

That's great news!

Is this weird?

Nope, not weird at all. I have several packages that do this.

We could also move to a new package, maybe import graphql from 'relay.macro'?

This is what I do. For example codegen.macro's entry file:

// the entire point of this package is to make it easier to use
// babel-plugin-codegen/macro
module.exports = require('babel-plugin-codegen/macro')

should babel-plugin-relay have some sort of dependency on babel-plugin-macros as there's a require in there?

Yes, if babel-plugin-relay publishes code that uses babel-plugin-macros then it should have babel-plugin-macros as a regular dependency.

kentcdodds commented Aug 8, 2018

That's great news!

Is this weird?

Nope, not weird at all. I have several packages that do this.

We could also move to a new package, maybe import graphql from 'relay.macro'?

This is what I do. For example codegen.macro's entry file:

// the entire point of this package is to make it easier to use
// babel-plugin-codegen/macro
module.exports = require('babel-plugin-codegen/macro')

should babel-plugin-relay have some sort of dependency on babel-plugin-macros as there's a require in there?

Yes, if babel-plugin-relay publishes code that uses babel-plugin-macros then it should have babel-plugin-macros as a regular dependency.

@kassens

This comment has been minimized.

Show comment
Hide comment
@kassens

kassens Aug 8, 2018

Member

@apalm want to merge the changes from kassens/relay@d7321f9 here? When I merge this it'll all be a single commit and you did most of the work :)

Member

kassens commented Aug 8, 2018

@apalm want to merge the changes from kassens/relay@d7321f9 here? When I merge this it'll all be a single commit and you did most of the work :)

@facebook-github-bot

kassens has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@kassens

This comment has been minimized.

Show comment
Hide comment
@kassens

kassens Aug 8, 2018

Member

Figured it out, hopefully not running into problems with the internal merge now!

Member

kassens commented Aug 8, 2018

Figured it out, hopefully not running into problems with the internal merge now!

@apalm

This comment has been minimized.

Show comment
Hide comment
@apalm

apalm Aug 8, 2018

Contributor

@kassens Thank you for picking this back up (and for fixing the issues with the original approach 😅).

I think the docs still need updating though, per the changes you mentioned in #2171 (comment).

I was also able to confirm a working CRA locally using react-scripts@2.0.0-next.3e165448 and using import graphql from "babel-plugin-relay/macro" :)

Contributor

apalm commented Aug 8, 2018

@kassens Thank you for picking this back up (and for fixing the issues with the original approach 😅).

I think the docs still need updating though, per the changes you mentioned in #2171 (comment).

I was also able to confirm a working CRA locally using react-scripts@2.0.0-next.3e165448 and using import graphql from "babel-plugin-relay/macro" :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment