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

[discussion] refactor api #32

Closed
yoshuawuyts opened this issue Jan 11, 2016 · 7 comments
Closed

[discussion] refactor api #32

yoshuawuyts opened this issue Jan 11, 2016 · 7 comments

Comments

@yoshuawuyts
Copy link
Contributor

supersedes #31

current status

sheetify's api is a bit all over the place. Currently we support:

  • js callback api
  • js stream api
  • command line
  • browserify transform (both js and package.json styles)

When sheetify was first written, it served as an alternative to LESS and SASS. The current version aims to provide namespaces as a browserify transform and import dependency resolution to arbitrary preprocessors (never fully implemented).

proposal

I propose we refactor the api to work optimally for the current goals. This way we can shrink the code base, provide a more coherent story to users and generally improve the usefulness of the tool.

The changes I'd like to see are:

  • drop the cli api - other preprocessors have cli's that can be used directly
  • drop the js callback and js stream api - other preprocessors have api's that can be used directly
  • add a way to trigger file watches from the transform (related --watch/-w option #21)
  • add a way to exorcise inline styles from the resulting browserify bundle
  • add support for inline template strings in addition to external files (related template strings for sheetify #31)
  • add support for including files that aren't namespaced (such as reset.css) (iirc @hughsk had ideas for this)

caveats

  • sheetify can no longer be used as a linker; this was never fully implemented but would no longer be possible
  • sheetify becomes dependent on browserify - imo that's a good thing as that is where the value is added. webpack support could be added through an external browserify -> webpack adapter

I hope I'm making sense with this all; I'd be happy to see sheetify progress down the path to simplification.

cc/ @hughsk @ahdinosaur I'd be keen to hear your thoughts on this

@hughsk
Copy link
Member

hughsk commented Jan 11, 2016

I'm about to go to sleep :) But quickly:

add support for inline template strings in addition to external files

This doesn't need to be specific to template strings, you could just do:

const prefix = sheetify(`
  /* Inline CSS */
`, { inline: true })

Then there's no need to depend on ES6 syntax, and you now have one API instead of two 🎉

@ahdinosaur
Copy link
Member

so keen, this is exactly where i'd like to go with sheetify. thanks for writing this up @yoshuawuyts. 👍

@yoshuawuyts
Copy link
Contributor Author

@hughsk I think that's a reasonable proposal; I agree we should adopt that syntax.

I'd be happy to see sheetify also support tagged template strings using require('sheetify') (I'm hoping babylon can pick them up), but I reckon that's more of a stretch goal so it's best to focus on the simplest version for now.

@ahdinosaur
Copy link
Member

from #16, might want to add the ability to use postcss plugins via the options, as opposed to a separate plugin format (for example, cssnext is now postcss-cssnext).

an open question is whether we specify any default plugins, such as what css-modulesify does. specifically i'm thinking of postcss-import, perhaps also a way to pass values around (like this but in our own way).

@yoshuawuyts
Copy link
Contributor Author

Yeah, I reckon directly consuming postcss plugins would be reasonable, given that we're using it as a parser we might as well go all the way. Despite the postcss plugin format being unnecessarily clunky, his saves us work of creating a compatibility layer. If we're going for simplification we might as well go with it; if for some reason postcss becomes irrelevant in the future we'll deal with a different plugin format then.

I'm cool with adding postcss-import as it's a good fit for sheetify and the idea of modularity / namespacing.

@yoshuawuyts
Copy link
Contributor Author

after talking to @hughsk in person, I have come to agree that using postcss-* style plugins directly isn't a good idea. I think we're all coming here from a "least maintenance needed" perspective, and while hooking into postcss-* might make it easier to interop with that ecosystem, not doing it reduces the the surface of stuff we should potentially care about.

I'm not sure I'm doing a great job at explaining this, but I hope it makes sense.

@yoshuawuyts
Copy link
Contributor Author

I've created separate issues for all todo's mentioned in this thread. I'm sure as we implement it all, new issues will arise. So I'm closing this issue for now as we've resolved this discussion into concrete actionables ✨

If I've missed anything tho, don't hesitate to reopen.

@ahdinosaur ahdinosaur mentioned this issue Feb 3, 2016
7 tasks
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

No branches or pull requests

3 participants