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

feat: add postcss-plugin #781

Merged
merged 5 commits into from
Nov 26, 2024
Merged

feat: add postcss-plugin #781

merged 5 commits into from
Nov 26, 2024

Conversation

javascripter
Copy link
Contributor

@javascripter javascripter commented Nov 24, 2024

What changed / motivation ?

  • Adds a PostCSS Plugin named @stylexjs/postcss-plugin as described in the linked issue.
  • Updates nextjs-example to use @stylexjs/postcss-plugin

Linked PR/Issues

Fixes #779

Additional Context

The implementation is mostly based on postcss-react-strict-dom that I wrote but have decided to convert it to JavaScript instead of TypeScript to make it suitable for this repository.

The plugin is rather simple so I didn't add Flow types for now (same as the current nextjs-plugin).

I've described the overview of this plugin approach here.

This PR:
Screenshot 2024-11-24 at 16 14 35
Original:
Screenshot 2024-11-24 at 16 14 58

Pre-flight checklist

apps/nextjs-example/app/stylex.css Outdated Show resolved Hide resolved
apps/nextjs-example/babel.config.js Show resolved Hide resolved
'app/**/*.{js,jsx,ts,tsx}',
'components/**/*.{js,jsx,ts,tsx}',
// TODO: Figure out a better way to write this
'./node_modules/@stylexjs/open-props/lib/**/*.{js,jsx,ts,tsx}',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use something like this initially

require.resolve('@stylexjs/open-props')

as suggested here but that didn't work because of individual package.json exports. For now I manually specified the files to transform and the example is rendering correctly.

Copy link
Contributor

@necolas necolas Nov 25, 2024

Choose a reason for hiding this comment

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

We shouldn't ever have to process non .js or .mjs files from node_modules packages, as everything gets compiled to js before publishing.

A more reliable way to get the package exports for any package (inc if they change their exports) is to use a script. Something like this:

const fs = require('fs');
const path = require('path');

function getPackageExports(packageName) {
  const packageJsonPath = require.resolve(`${packageName}/package.json`);
  const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8'));

  if (packageJson.exports) {
    const exports = {};
    for (const key in packageJson.exports) {
      if (typeof packageJson.exports[key] === 'string') {
        exports[key] = require.resolve(path.join(packageName, packageJson.exports[key]));
      } else if (typeof packageJson.exports[key] === 'object') {
        exports[key] = getPackageExports(path.join(packageName, key));
      }
    }
    return exports;
  } else {
    return require.resolve(packageName);
  }
}

const openPropsExports = getPackageExports('@stylexjs/open-props');

Copy link
Contributor Author

@javascripter javascripter Nov 25, 2024

Choose a reason for hiding this comment

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

Currently,

require.resolve('@stylexjs/open-props/package.json')

throws this error:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './package.json' is not defined by "exports" in /Users/tsubasa/Projects/github.com/javascripter/stylex/apps/nextjs-example/node_modules/@stylexjs/open-props/package.json

This is the postcss.config.js I tried and I found some comments about this:
react-native-community/cli#1168 (comment)

Please don't open issues/PRs on open source projects about this. It is not expected that packages export their package.json file. This is a problem React Native needs to fix, not every single package out there.

I'm having a difficulty finding a reliable way to get package.json export paths or even just the root of the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated open-props include paths so that it no longer hard-code internal paths (e.g. /lib) in 378c80e
Not as accurate as I'd like as it doesn't look for exports but it now accounts for node_modules hoisting and should be more stable and explicit about what it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://nodejs.org/api/packages.html#package-entry-points

Existing packages introducing the "exports" field will prevent consumers of the package from using any entry points that are not defined, including the package.json (e.g. require('your-package/package.json')). This will likely be a breaking change. To make the introduction of "exports" non-breaking, ensure that every previously supported entry point is exported. It is best to explicitly specify entry points so that the package's public API is well-defined.

It's strange that package.json isn't reachable by default, seems like a Node.js issue tbh. But we can export package.json from packages we control

apps/nextjs-example/postcss.config.js Show resolved Hide resolved
packages/postcss-plugin/src/bundler.js Show resolved Hide resolved
Copy link
Contributor

@nmn nmn left a comment

Choose a reason for hiding this comment

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

This looks really good and much simpler than I thought it would be.

Thank you so much!

apps/nextjs-example/app/stylex.css Outdated Show resolved Hide resolved
apps/nextjs-example/babel.config.js Show resolved Hide resolved
apps/nextjs-example/postcss.config.js Show resolved Hide resolved
Comment on lines +48 to +52
include: [
'app/**/*.{js,jsx,ts,tsx}',
'components/**/*.{js,jsx,ts,tsx}',
...openPropsIncludePaths,
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so I can verify how the postcss plugin works:

We're fining all files that match these globs and compiling them as a list.

Since we're reading all the files manually anyway, if we wanted to we could accept a list of "entrypoints" instead and resolve the dependency graph, right?

Copy link
Contributor Author

@javascripter javascripter Nov 26, 2024

Choose a reason for hiding this comment

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

Since we're reading all the files manually anyway, if we wanted to we could accept a list of "entrypoints" instead and resolve the dependency graph, right?

Yes. The postcss plugin globs and compiles all matching files to extract css.
This means we may be over-compiling in some cases similar to how Tailwind CLI does it with the content glob.

We can accept entry points and resolve dependency graphs ourselves but we need to take into account aliases, configured platform extensions (.web.ts), and so on in some cases to replicate bundler dependency graphs so I avoided that to allow greater flexibility to users and to be bundler-agnostic in general.

Dependency graph resolvers are often a source of subtle bugs that aren't easy for users to fix unless there's an escape hatch, so I wrote getPackageIncludePaths function in postcss.config.js instead of integrating it within the postcss plugin so users can adjust it for their setup manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this all makes sense. Thanks!

}

// Transforms the source code using Babel, extracting StyleX rules and storing them.
async function transform(id, sourceCode, babelConfig, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could share some code between the postcss plugin and the CLI. Although this plugin should make the CLI redundant in most cases.

Copy link
Contributor Author

@javascripter javascripter Nov 26, 2024

Choose a reason for hiding this comment

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

The main goal of this PR is to make the plugin simple, reliable and production-ready for Next.js and other bundler users so I made the plugin simple and self-contained.

It's out of the scope of this PR but I have some success in making this work for Next.js + Turbopack setup as well (gist). It depends on some internals and am not using it in my production setup but I see a path where StyleX CLI becomes unnecessary for Turbopack users in the future too so it's worth keeping this plugin decoupled from the CLI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just mean we can refactor the CLI to share some of the code between it and the postcss plugin. Anyway, definitely out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @javascripter's point is that it's simpler keeping the packages decoupled. There's no real value to sharing code here

@nmn nmn merged commit b9441d6 into facebook:main Nov 26, 2024
7 of 8 checks passed
@nmn
Copy link
Contributor

nmn commented Nov 26, 2024

Thank you so much @javascripter! This will massively improve the experience of using StyleX.

@javascripter javascripter deleted the postcss-plugin branch November 27, 2024 08:29
aminaopio pushed a commit to aminaopio/stylex that referenced this pull request Dec 22, 2024
* feat: add postcss-plugin
* fix postcss-plugin readme text
* remove .ts, .tsx from node_modules @stylexjs/open-props globs in postcss.config.js
* do not rely on internal exports path
* fix: remove redundant @layer stylex
aminaopio pushed a commit to aminaopio/stylex that referenced this pull request Dec 22, 2024
* feat: add postcss-plugin
* fix postcss-plugin readme text
* remove .ts, .tsx from node_modules @stylexjs/open-props globs in postcss.config.js
* do not rely on internal exports path
* fix: remove redundant @layer stylex
aminaopio pushed a commit to aminaopio/stylex that referenced this pull request Dec 22, 2024
* feat: add postcss-plugin
* fix postcss-plugin readme text
* remove .ts, .tsx from node_modules @stylexjs/open-props globs in postcss.config.js
* do not rely on internal exports path
* fix: remove redundant @layer stylex
aminaopio pushed a commit to aminaopio/stylex that referenced this pull request Dec 22, 2024
* feat: add postcss-plugin
* fix postcss-plugin readme text
* remove .ts, .tsx from node_modules @stylexjs/open-props globs in postcss.config.js
* do not rely on internal exports path
* fix: remove redundant @layer stylex
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bundlers] Investigate creating a postCSS plugin to create more reliable Bundler integrations
4 participants