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

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

Open
tmeindle opened this issue Jan 30, 2024 · 9 comments
Open

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

tmeindle opened this issue Jan 30, 2024 · 9 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@tmeindle
Copy link

tmeindle commented Jan 30, 2024

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 tmeindle added the bug Something isn't working label Jan 30, 2024
@nmn
Copy link
Contributor

nmn commented Jan 31, 2024

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.

@tmeindle
Copy link
Author

tmeindle commented Jan 31, 2024

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

@nmn
Copy link
Contributor

nmn commented Feb 1, 2024

@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 nmn closed this as completed Feb 1, 2024
@tmeindle
Copy link
Author

tmeindle commented Feb 1, 2024

@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

@nmn
Copy link
Contributor

nmn commented Feb 2, 2024

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.

@tmeindle
Copy link
Author

tmeindle commented Feb 2, 2024

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

@nmn
Copy link
Contributor

nmn commented Feb 3, 2024

@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 nmn reopened this Feb 3, 2024
@nmn nmn added the help wanted Extra attention is needed label Feb 3, 2024
@tmeindle
Copy link
Author

tmeindle commented Feb 5, 2024

@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

@nmn
Copy link
Contributor

nmn commented Feb 7, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants