Skip to content
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

Optionally output eager es modules #2781

Closed
wants to merge 4 commits into from

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Jun 24, 2019

In this PR I added eagerESModules option to relay-runtime and babel-plugin-relay. I have two problem which can be solved with this change

  1. Rollup handles only es modules. To handle commonjs there is a plugin
    which converts it into esm. Though it always skips module if
    import/export statements are found. As a result I get runtime error:
    "require is not defined".

To workaround this I wrote custom plugin. Though it would be cool to
have proper solution out of the box.

{
  name: 'relay-generated',
  transform(code) {
    // convert __generated__ requires into imports
    // remove after relay-compiler will be able to emit esm
    if (code.includes('__generated__')) {
      let i = -1;
      const paths = [];
      const processed = code.replace(
        /require\((.+__generated__.+)\)/g,
        (req, pathString) => {
          i += 1;
          paths.push(pathString);
          return `__generated__${i}`;
        },
      );
      const imports = paths.map(
        (p, i) => `import __generated__${i} from ${p};\n`,
      );
      return {
        code: imports.join('') + processed,
      };
    }
  },
},
  1. Another problem is flow bug (import type allows not existent types, despite untyped-type-import being enabled flow#7444) which treats all not existent types as any.
    This leads to many type unsafe places.
import type {NotExistentType} from './__generated__/MyQuery.graphql';

type Props = {|
  my: NotExistentType
|}

@TrySound
Copy link
Contributor Author

Hi @josephsavona. What do you think about this change?

@TrySound
Copy link
Contributor Author

TrySound commented Jul 7, 2019

Hi @kassens Could you take look?

@dousaitis
Copy link

This will be very useful for people who use rollup to bundle their app.

@frenzzy
Copy link

frenzzy commented Jul 29, 2019

PR resolves the following issue: #2445

P.S.: I linked the related issue to help people like me find a solution and of course would be great to get this feature accepted.

@TrySound
Copy link
Contributor Author

TrySound commented Aug 3, 2019

Friendly ping @kassens @josephsavona

@TrySound
Copy link
Contributor Author

TrySound commented Aug 7, 2019

/cc @jstejada

@dousaitis
Copy link

Any update on this?

@TrySound
Copy link
Contributor Author

@kassens Do you mind to review this PR? Any changes required to move it forward?

@sgwilym
Copy link
Contributor

sgwilym commented Aug 27, 2019

This would be a really great addition.

@frenzzy
Copy link

frenzzy commented Sep 4, 2019

I just published this feature under @frenzzy/babel-plugin-relay.
I use the feature to run my app in browsers with es6 support without bundling to speed up dev process and I hope it may help someone else too.

adeira-github-bot pushed a commit to adeira/relay-example that referenced this pull request Sep 9, 2019
Inspired by:

- facebook/relay#2445
- facebook/relay#2781
- facebook/relay#2841

kiwicom-source-id: a0181e85ec323a361502ef8275739cf18ef114ac
adeira-github-bot pushed a commit to kiwicom/relay that referenced this pull request Sep 9, 2019
Inspired by:

- facebook/relay#2445
- facebook/relay#2781
- facebook/relay#2841

kiwicom-source-id: a0181e85ec323a361502ef8275739cf18ef114ac
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@TrySound
Copy link
Contributor Author

Hi @jstejada. Any news on this? Do you want me to assist with something?

@TrySound
Copy link
Contributor Author

Friendly ping @jstejada

@TrySound
Copy link
Contributor Author

Friendly ping @jstejada @kassens @alunyov. Do you need me to rebase this PR or you have something internally?

@dousaitis
Copy link

Hi @frenzzy! Could you please add the latest version 8.0.0 to @frenzzy/babel-plugin-relay as this MR still remains open and we can't bundle with rollup at all...

@jstejada @kassens @alunyov could you please merge this MR? It's open from the summer!!

@frenzzy
Copy link

frenzzy commented Dec 24, 2019

I would like to release v8, but need to resolve conflicts first, it is quite complex now after so radical changes. @TrySound can you please update your PR (rebase on master)?

@TrySound
Copy link
Contributor Author

im out of computer til 5 jan, sorry

@frenzzy
Copy link

frenzzy commented Dec 24, 2019

Just released @frenzzy/babel-plugin-relay v8.0.0, hope I rebased correctly (at least tests are passing). You can see the diff here: frenzzy@4d485bc
@TrySound thank you and enjoy your holiday :)

I have two problem which can be solved with this change

1. Rollup handles only es modules. To handle commonjs there is a plugin
which converts it into esm. Though it always skips module if
import/export statements are found. As a result I get runtime error:
"require is not defined".

To workaround this I wrote custom plugin. Though it would be cool to
have proper solution out of the box.

```js
{
  name: 'relay-generated',
  transform(code) {
    // convert __generated__ requires into imports
    // remove after relay-compiler will be able to emit esm
    if (code.includes('__generated__')) {
      let i = -1;
      const paths = [];
      const processed = code.replace(
        /require\((.+__generated__.+)\)/g,
        (req, pathString) => {
          i += 1;
          paths.push(pathString);
          return `__generated__${i}`;
        },
      );
      const imports = paths.map(
        (p, i) => `import __generated__${i} from ${p};\n`,
      );
      return {
        code: imports.join('') + processed,
      };
    }
  },
},
```

2. Another problem is flow bug (facebook/flow#7444) which treats all not existent types as any.
This leads to many type unsafe places.

```js
import type {NotExistentType} from './__generated__/MyQuery.graphql';

type Props = {|
  my: NotExistentType
|}
```
@TrySound
Copy link
Contributor Author

@jstejada @kassens @alunyov @josephsavona I updated esm output considering recent thunks removal. Tests are much smaller now too.

@TrySound
Copy link
Contributor Author

Friendly ping @jstejada @kassens @josephsavona

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@TrySound
Copy link
Contributor Author

Yay!

@facebook-github-bot
Copy link
Contributor

@tyao1 merged this pull request in db7a661.

jstejada pushed a commit that referenced this pull request Feb 13, 2020
Summary:
In this PR I added `eagerESModules` option to `relay-runtime` and `babel-plugin-relay`. I have two problem which can be solved with this change

1. Rollup handles only es modules. To handle commonjs there is a plugin
which converts it into esm. Though it always skips module if
import/export statements are found. As a result I get runtime error:
"require is not defined".

To workaround this I wrote custom plugin. Though it would be cool to
have proper solution out of the box.

```js
{
  name: 'relay-generated',
  transform(code) {
    // convert __generated__ requires into imports
    // remove after relay-compiler will be able to emit esm
    if (code.includes('__generated__')) {
      let i = -1;
      const paths = [];
      const processed = code.replace(
        /require\((.+__generated__.+)\)/g,
        (req, pathString) => {
          i += 1;
          paths.push(pathString);
          return `__generated__${i}`;
        },
      );
      const imports = paths.map(
        (p, i) => `import __generated__${i} from ${p};\n`,
      );
      return {
        code: imports.join('') + processed,
      };
    }
  },
},
```

2. Another problem is flow bug (facebook/flow#7444) which treats all not existent types as any.
This leads to many type unsafe places.

```js
import type {NotExistentType} from './__generated__/MyQuery.graphql';

type Props = {|
  my: NotExistentType
|}
```
Pull Request resolved: #2781

Reviewed By: jstejada

Differential Revision: D17385417

Pulled By: tyao1

fbshipit-source-id: dfa031412666b8afcac4cbae4678f64abbbe5ec9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants