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: support postcss sugarss #2436

Closed
wants to merge 2 commits into from
Closed

Conversation

piecyk
Copy link

@piecyk piecyk commented Mar 8, 2021

Hi, this PR adds support for handling https://github.com/postcss/sugarss in css plugin, it's Indent-based CSS syntax for PostCSS.

Biggest change here is how the postcss config is resolve.

For handling .sss we need tell postcss to use the sugarss parser but not for css files. This can be achieved via passing a id that will be passed ctx: { file: string } to config. Similar as https://github.com/webpack-contrib/postcss-loader does it.

Current implementation was caching resolved config, this is not true now as we resolved it per id. Changed it to keep cache for each id, but wondering now if that will give any significant performance improvement, or just increase memory 🤔

Added cachedPostcssConfigPath that will keep path to resolved config this can impact performance.

@piecyk
Copy link
Author

piecyk commented Mar 9, 2021

Ok, changed to only cache postcss config path 😄

plugins: [require('postcss-nested')]
module.exports = (api) => {
if (/\.sss$/.test(api.file)) {
return {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick (non-blocking): I would prefer an initial object at the start of the function body and return it at the end of the function
Then in between you can add dynamically properties and extend it

So something like

  const config = {
    plugins: [require('postcss-nested')]
  }

  if (/\.sss$/.test(api.file)) {
    config.parser = 'sugarss'
    config.plugins.unshift(require('postcss-simple-vars'))
  }

  return config

May need to be extended with some type-safety or so

} catch (e) {
if (!/No PostCSS Config found/.test(e.message)) {
throw e
}
return (cachedPostcssConfig = null)
return (cachedPostcssConfigPath = null)
Copy link
Member

Choose a reason for hiding this comment

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

thought (dx): I'm a bit confused by this line
Could we split it into two lines?
I think it means the same as 🤔

cachedPostcssConfigPath = null
return null

@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Mar 23, 2021
@piecyk
Copy link
Author

piecyk commented Mar 31, 2021

@yyx990803 what do you think this postcss sugarss ?

(typeof inlineOptions === 'string' ? inlineOptions : config.root)

const ctx = { file, env: config.mode }
const result = (await postcssrc(ctx, searchPath)) as PostCSSConfigResult
Copy link
Member

Choose a reason for hiding this comment

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

This is obviously very inefficient - I think a better way to do this is have two cached & resolved config values - one for SugarSS and one for normal CSS.

Copy link
Member

Choose a reason for hiding this comment

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

We can cache the resolved config with a hash of the ctx object, but we'll need to replace ctx.file with ctx.type (the file extension without the dot).

Copy link
Member

Choose a reason for hiding this comment

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

Another option is to provide a cache function like Babel does it:
https://babeljs.io/docs/en/config-files#apicache

@piecyk piecyk closed this Dec 7, 2021
@piecyk piecyk deleted the feat/css/add-sugarss branch December 7, 2021 16:23
@vitalybaev
Copy link
Contributor

@piecyk is it possible to get this merged? Or maybe I'd use parts of your work?

@patak-dev
Copy link
Member

@vitalybaev you can fork this PR and keep working on it to address the issues listed above 👍🏼

@vitalybaev vitalybaev mentioned this pull request Jan 31, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants