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

[nextjs] Update the withPigment API to accept config #114

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented May 30, 2024

through pigment key from the nextConfig object itself instead of as a 2nd argument to the call. This is a standard approach followed by other nextjs plugins

Fixes: #83
Fixes: #88

@brijeshb42 brijeshb42 added enhancement This is not a bug, nor a new feature nextjs labels May 30, 2024
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I am 👌 with this change, we just have to make sure we document it as a breaking change in the changelog

@brijeshb42
Copy link
Contributor Author

brijeshb42 commented Jun 17, 2024

I'd say it deprecates the current API as it can still take the input as second argument. But I'll update the docs to note this.

@brijeshb42 brijeshb42 requested a review from mnajdova June 17, 2024 09:44
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 17, 2024
through `pigment` key from the nextConfig object itself instead of as a
2nd argument to the call. This is a standard approach followed by other
nextjs plugins

Fixes: mui#83
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 21, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 2, 2024
@siriwatknp
Copy link
Member

I am dropping my thought here based on my user expectation and writing docs.

I would like all bundler plugin to use the same name, e.g. createPigmentPlugin:

Next.js

import { createPigmentPlugin } from '@pigment-css/nextjs-plugin';

const pigmentPlugin = createPigmentPlugin(…pigment config);

export default pigmentPlugin(…next config)

Vite:

import { createPigmentPlugin } from '@pigment-css/vite-plugin';

const pigmentPlugin = createPigmentPlugin(…pigment config);

export default defineConfig({
  plugins: [pigmentPlugin()]
})

With the above signature, writing docs will be a lot easier because we can refer to the setup once and later refer to only "pigment config" for other feature.

@brijeshb42
Copy link
Contributor Author

brijeshb42 commented Jul 11, 2024

Seems like in latest versions of Next.js, it warns users when it finds an unknown key in the config

 ⚠ Invalid next.config.mjs options detected: 
 ⚠     Unrecognized key(s) in object: 'pigment'
 ⚠ See more info here: https://nextjs.org/docs/messages/invalid-next-config

Now, I am not so sure that we should do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement This is not a bug, nor a new feature nextjs PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[examples] Update Next.js example to use ESM How can I custom a theme with NX+Next.js?
3 participants