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

Add support for plugins #190

Closed
wants to merge 10 commits into from
Closed

Add support for plugins #190

wants to merge 10 commits into from

Conversation

giuseppeg
Copy link
Collaborator

Fixes #182

@giuseppeg giuseppeg requested review from rauchg and nkzawa May 13, 2017 14:21
}
```

In order to resolve local plugins paths you can use NodeJS' [require.resolve](https://nodejs.org/api/globals.html#globals_require_resolve).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be nice if we could find a way to magically resolve paths internally.
If it is not possible then asking them to use require.resolve or anyway provide full paths is ok I guess.

### Options

* `sourceMaps` generates source maps (default: `false`)
* `vendorPrefix` turn on/off automatic vendor prefixing (default: `true`)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should this be a noun/plural? vendorPrefixes

(css, settings) => { /* ... */ }
```

The `settings` object has the following shape:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this object could as well be renamed to options and change shape:

{
  // user options
  exampleOption: true,

  // babel options
  babel: { // or env
    sourceMaps: true,
    vendorPrefix: true,
  }
}

@giuseppeg giuseppeg mentioned this pull request Jun 4, 2017
@giuseppeg giuseppeg force-pushed the add-plugins-support branch 2 times, most recently from ccddb77 to 96fd965 Compare June 11, 2017 08:30
the babel and user options:

```js
(css, settings) => { /* ... */ }
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it support async operations ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no it doesn't because of the sync nature of babel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fwiw in styled-jsx-postcss I use a super hack to make async code work (syncronously) https://github.com/giuseppeg/styled-jsx-postcss/blob/master/src/babel.js#L98-L107

@nkzawa
Copy link
Contributor

nkzawa commented Jun 12, 2017

I wonder if we can implement the example of #5. (can you get type="text/sass" ?)

Additionally, I wonder if there are any limitations comparing to creating a babel plugin which transforms css string before styled-jsx compilation runs.

@giuseppeg
Copy link
Collaborator Author

giuseppeg commented Jun 12, 2017

I wonder if we can implement the example of #5. (can you get type="text/sass" ?)

We could but I'd rather keep those extensions in user space otherwise we'd have to ship styled-jsx with node-sass, postcss etc

Additionally, I wonder if there are any limitations comparing to creating a babel plugin which transforms css string before styled-jsx compilation runs.

Definitely an option but obviously writing a simple function is easier than setting up a new babel plugin that has to detect <style jsx> tags, replace expressions with placeholders, preprocess the string and then replace the original template literal. Also with external styles they'd need to detect css exports like we do in order to apply plugins to external css.

@nkzawa
Copy link
Contributor

nkzawa commented Jun 12, 2017

I'm rather wondering if you can get props of <style jsx/>. I totally agree this style of plugins would be better.

Can we have props as an argument (or in settings) ?

export default (css, props, settings) => {
   if (props.type === 'text/sass') {
     return compileSass(css);
   } else {
     return css;
   }
}

@giuseppeg
Copy link
Collaborator Author

Can we have props as an argument (or in settings) ?

ah yes I like the idea but unfortunately we wouldn't have those info for external stylesheets :/

@giuseppeg
Copy link
Collaborator Author

@nkzawa ideally people can annotate css eg:

<style jsx>{`
  /* @styled-jsx-sass */
  div {
    color: red;
    &:hover { color: blue }
  } 
`}</style>

and the plugin author can check for that comment

export default (css, props, settings) => {
   if (/@styled-jsx-sass/.test(css)) {
     return compileSass(css);
   } else {
     return css;
   }
}

@giuseppeg giuseppeg force-pushed the add-plugins-support branch from 96fd965 to dbe522a Compare June 19, 2017 18:46
@giuseppeg
Copy link
Collaborator Author

giuseppeg commented Jun 20, 2017

another thing to do is figure out whether this feature could make dynamic styles (#81) harder to implement in the future

@Tomekmularczyk
Copy link

what is the status of this pull request?

@giuseppeg
Copy link
Collaborator Author

update: we decided to wait until we implement #81 since it might affect how plugin works.

@medv
Copy link

medv commented Aug 30, 2017

When this gets merged I'm selling all my stuff and moving to styled-jsx island.

@jabacchetta
Copy link
Contributor

Also very interested in both this and #81. Trying to decide between styled-jsx or styled-components with Next.js, and these seem to be the missing pieces.

@smeijer
Copy link

smeijer commented Sep 6, 2017

@giuseppeg update: we decided to wait until we implement #81 since it might affect how plugin works.

So, what's the status of #81? Since I really like to have these plugins available, to be able to parse PostCSS.

@giuseppeg
Copy link
Collaborator Author

It is in a good spot, in my little spare time I am trying to finish #81 basically there are a few minor adjustments to do, need to write extensive tests and docs. You can track the progress here https://github.com/zeit/styled-jsx/tree/dynamic-styles

@Faradey27
Copy link

Hi, do we plan to merge this pull request?(and resolve conflicts)

@giuseppeg
Copy link
Collaborator Author

@Faradey27 finishing up #81 and planning on updating this PR and get it merged. With the limited amount of free time that I have it might take a few weeks 😅 but I will do my best

@giuseppeg
Copy link
Collaborator Author

Duplicate of #291
Reworked #291

@giuseppeg giuseppeg closed this Oct 1, 2017
@giuseppeg giuseppeg deleted the add-plugins-support branch November 21, 2017 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants