stylex icon indicating copy to clipboard operation
stylex copied to clipboard

Babel plugin is not compatible with @babel/preset-env

Open tmeindle opened this issue 2 years ago • 9 comments

Describe the issue

When using rollup plugin, stylex.css stops being output when @babel/preset-env is added to a babel config in root directory

Expected behavior

If the plugin is supposed to be compatible with @babel/preset-env, I would expect the stylex.css to still be output after adding @babel/preset-env

Steps to reproduce

  1. Use rollup-example as a starting point (I copied this folder out of npm workspace).
  2. Add the latest versions of rollup, @babel/cli, @babel/core, @babel/preset-env to package.json dependencies
  3. npm install using npm 18.15.0
  4. npm run build (works at this point and stylex.css is output)
  5. add a babel-config.js (or json) that includes @babel/preset-env in the presets array
{
  "presets": [
    [
      "@babel/preset-env",
      {
        "targets": {
          "node": "18"
        }
      }
    ]
  ]
}

And then styles stop working. Seems any other plugins or presets works but the minute I add preset-env no styles are generated

Test case

No response

Additional comments

No response

tmeindle avatar Jan 30 '24 20:01 tmeindle

You need to ensure that you don't transform import statements before StyleX is done. Instead of using a separate babebl plugin, you can pass your babel config to the StyleX rollup plugin and things should work correctly.

nmn avatar Jan 31 '24 04:01 nmn

So basically I will have to transpile 2x if I want to use preset-env or set preset-env's modules option to false which sorta of defeats the purpose of using it. How else would I ensure that the stylex runs before preset-env?

rollup-example.zip

tmeindle avatar Jan 31 '24 14:01 tmeindle

@tmeindle The issue is that what you're trying to do isn't really compatible with Rollup either.

In your example, uncomment the modules: false, line so that preset-env leaves the import statements alone. Then, change output.format of the Rollup config from esm to cjs. That will give you a cjs output after bundling with Rollup.

nmn avatar Feb 01 '24 06:02 nmn

@tmeindle The issue is that what you're trying to do isn't really compatible with Rollup either.

In your example, uncomment the modules: false, line so that preset-env leaves the import statements alone. Then, change output.format of the Rollup config from esm to cjs. That will give you a cjs output after bundling with Rollup.

@nmn

But.... I am not trying to output cjs modules. I am trying to output es modules. I'm pretty sure it is still a bug somewhere in stylex plugins (babel or rollup)

@babel/preset-env with target of node18 outputs es module syntax that are compatible with node 18 when used with @rollup/plugin-babel.

Rollups output format is set to es so the output should be esm modules. If you set rollup output to cjs, rollup will convert the outputted modules to cjs with some added es interop. I should be able to build both a cjs and esm version with rollup in this way. This all works fine with @rollup/plugin-babel. I have been using a setup like this for at least 5 years.

When I use @babel/preset-env with target of node18 with the @stylex/rollup-plugin and no other changes. (i.e. the setup I sent you without the "modules: false" setting uncommented) the output for the js bundle from rollup is in cjs when it should be in esm and no styles are output. If I change the rollup output type the cjs and leave "modules: false" commented out I get the same result.

But uncommenting modules: false makes everything work as expected. However this is a work around that should not be needed.

https://babeljs.io/docs/babel-preset-env.html#modules "Setting this to false will preserve ES modules. Use this only if you intend to ship native ES Modules to browsers. If you are using a bundler with Babel, the default modules: "auto" is always preferred."

image

Maybe the "caller" babel option is being set incorrectly (or not at all) so that in turn the default modules: "auto" option doesn't work correctly.

https://babeljs.io/docs/options#caller

see @rollup/plugin-babel https://github.com/rollup/plugins/blob/2a19079892f0bef53b557da965339cdef0a13a93/packages/babel/src/index.js#L78

tmeindle avatar Feb 01 '24 15:02 tmeindle

But uncommenting modules: false makes everything work as expected.

There is a idiosyncrasy with Rollup here. I tried removing all of StyleX and I still ran into issue without the modules: false. It might be an issue of using rollup/node-require or something.

Maybe the "caller" babel option is being set incorrectly (or not at all)

The StyleX plugin does not set the caller for you.

nmn avatar Feb 02 '24 00:02 nmn

The StyleX plugin does not set the caller for you.

That is exactly what the issue is here. According to the babel documentation, the stylex plug-in should be setting the caller option when it calls the async transform so that it knows the capabilities of rollup and can use it to configure the other babel plugins like preser-env automatically. It should be set the exact same way that the rollup babel plugin does it

tmeindle avatar Feb 02 '24 01:02 tmeindle

@tmeindle Re-opening the issue as there seems to be a simple fix to make the Rollup plugin more robust. I'm still not entirely sure what needs to be done, so would love some help here. I would really appreciate a PR if you know what needs to be done.

nmn avatar Feb 03 '24 01:02 nmn

@tmeindle Re-opening the issue as there seems to be a simple fix to make the Rollup plugin more robust. I'm still not entirely sure what needs to be done, so would love some help here. I would really appreciate a PR if you know what needs to be done.

Should just need to add:

caller: {
  name: '@stylex/rollup-plugin',
  supportsStaticESM: true,
  supportsDynamicImport: true,
  supportsTopLevelAwait: true,
  supportsExportNamespaceFrom: !this.meta.rollupVersion.match(/^1\.2[0-5]\./),
}

after line 98 here: https://github.com/facebook/stylex/blob/main/packages/rollup-plugin/src/index.js#L98

tmeindle avatar Feb 05 '24 22:02 tmeindle

@tmeindle Happy to accept and merge a PR, but I'll try it on my end soon if you're not interested in doing that.

Thanks for the proposed solution either way.

nmn avatar Feb 07 '24 04:02 nmn