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

On synchronous use and filtering styles #187

Closed
dantman opened this issue Dec 14, 2015 · 9 comments
Closed

On synchronous use and filtering styles #187

dantman opened this issue Dec 14, 2015 · 9 comments

Comments

@dantman
Copy link
Contributor

dantman commented Dec 14, 2015

One feature I've considered trying to embed in juice is the ability to filter and replace some of the style tags. For example taking all <style type="text/less"> tags, compiling them with less, and updating the content and type of the tag. Or filtering css through PostCSS.

Initially I thought it would be as simple as a patch like this. Which should work. But only for synchronous things, which is a problem since some notable css transpilers like less are async.

This brought up some more issues/points that merit discussion:

  • In some situations like an @import in LESS that was included through a <link> rather than an inline <style> tag the transpiler should know the original filename. This can be fixed either with another internal data- tag or by doing the filtering in web-resource-inliner (however the latter would mean filters cannot be applied to <style> tags, implementation would also be messier).
  • Is there still value in keeping the sync/async hybrid approach in juice if it makes the internal code more complex?
  • Rather than a styleFilter function, perhaps a plugin archatecture similar to what babel, broserify, and other packages have would be better. So pulgins can be written for things like less and sass.

Thoughts?

@jrit
Copy link
Collaborator

jrit commented Dec 14, 2015

In the most basic implementation, I think that LESS and others could be handled in the methods that also use web-resource-inliner. The other method signatures are basically for content that is ready to be directly inlined from the <style> contents into the document.

Since it is really web-resource-inliner that is handling pulling in stylsheets and making all the transforms outside just inlining CSS, I think handling external LESS docs probably belongs on web-resource-inliner instead of directly on Juice. The process of pulling in those resources happens in web-resource-inliner, juice doesn't look at particular external resources itself.

The processing/filtering of non-css <style> tag contents also seems useful, but it would be better to see that as an external dependency that just does a round of preprocessing on the <style> tags before Juice inlines the CSS. That could be used from within Juice similarly to how web-resource-inliner works I suppose, where the config just gets passed along.

A plugin architecture seems like an appropriate solution for attaching processors to files that are being pulled in and for transforming <style> content.

@dantman
Copy link
Contributor Author

dantman commented Dec 14, 2015

I understand the difference between sync and async methods in what would be inlined. The difficulty in implementing it in juice was that getStylesData is where the processing is done and it's a sync function. From that point to continue the hybrid approach of sync and async methods it's conceivable that a bunch of the internal methods might need to be duplicated to cover sync and async separately.

Doing it in web-resource-inliner (WRI) would be alright. The access to file context is better there. It would be a little disappointing not being able to <style type="text/less">...</style> but understandable.

The caveat is that there's a messy level of difficulty in doing it in WRI. Juice uses cheerio and so passing off an attributes object to a filter function or plugin and then manipulating parts of the element is easy. But WRI uses regular expressions to "parse" out the tags that need to be modified and does string replacements.

Either each of tag's attributes needs to be custom parsed in WRI so attribute data can be provided. Or WRI would need to be rewritten to use cheerio as well.

@jrit
Copy link
Collaborator

jrit commented Dec 14, 2015

I was more suggesting doing the processing of any LESS, etc. before anything gets to getStylesData, so that shouldn't need to change. So as an example transform pipeline:

WRI -> LESS and other transfoms -> Juice inlining with get StylesData

This assumes WRI will inline LESS which I don't remember at the moment, but would be a simple change if it doesn't. Using this approach, you could make a new npm module to handle the LESS and transforms, which could be useful on its own and would easily plug into Juice, similar to WRI. The 2nd stage could simply transform LESS or whatever in-place to CSS.

Sorry about my earlier explanation being unclear.

Do you think that would work?

@dantman
Copy link
Contributor Author

dantman commented Dec 14, 2015

Yeah, that idea might work.

@dantman
Copy link
Contributor Author

dantman commented Dec 20, 2015

On second pass of that idea. The transforms in that pipeline run after WRI has turned <link> into <style>. So we still need a way to keep filename information for the transforms.

I actually ran into this exact problem in my workaround to this issue. My workaround was to run WRI myself, create my own cheerio document, do my processing on the nodes in that document, then juice.juiceDocument the cheerio document and return the html.

But since WRI already inlined the <link> tags I have no context of where the styles were sourced from and I cannot set the correct paths option for less to allow the correct imports. I also cannot eaily pre-process those <link> tags with cheerio because WRI expects a string. So my workarounds are either regular expressions or the performance hit of parsing html and turning it back to text twice.

To handle this need for context in that pipeline which of these options do you like?

  • A data-filename (or other name) attribute (perhaps using resolved filenames if non-urls are used; since I'm not sure if the pipelines would know the right relative context)
  • data-link-* attributes for any attribute that WRI removes from a <link>, meaning the dropped href would become data-link-href. (May need to pass some of WRI's context data to the pipeline, but that's just relativeTo rather than context for each individual link)
  • A pre-WRI part of the pipeline "Pre-WRI transforms -> WRI -> Post-WRI transforms -> Juice inlining". Plugins like less that really need info from the could mess with things here. The caveat is this might be a bit of a mess when multiple plugins are basically trying to add pre-WRI transforms all to do the exact same thing. (This one requires WRI to use cheerio)

@jrit
Copy link
Collaborator

jrit commented Dec 24, 2015

Another option would be that WRI support transforming content in it's processing. That could maybe work by supplying a map of file types or content types to transforms. So, if WRI finds a *.less file or a tag with a type of text/less and has been configured with transforms then it will run the transform either before inserting the CSS into the document <style> (or transformed JS into <script>) or as a replacement when defined by the content type. That should I think make it unnecessary to track the prior states using attributes.

So that pipeline would then be

WRI [ run transforms internally ] -> Juice inlining with get StylesData

That would have the benefit then of WRI doing more of what it should be doing, which is inlining content in-place into HTML docs, and Juice continuing to focus on taking CSS and inlining it into <style> tags. It seems cleaner to me.

Some psuedo config for WRI might be like this, though this likely needs some more thought to get the details of this right.

transforms: [
  {
    extensions: [ "less" ]
    types: [ "text/less" ],
    transforms: ( doc, done ) => require( "less" ).render( doc, (e, output) => done( e, output.css ) )
  },
  ...
]

transforms in the above is just a function, so at long as the final callback includes the end result, you can do whatever you want. I have not fully through through what doc is here, maybe the file path and contents are properties? WRI may need to be changed a bit to accommodate all this, but if that requires breaking changes and a new major release, that could be ok. WRI could potentially have some transform presets as well so users don't have to always figure out how to write the LESS transform.

Is this something you'd considered or see problems with? Thanks for thinking through this with me.

@jrit
Copy link
Collaborator

jrit commented Jan 16, 2016

@dantman I was wondering if you are working on this. I've been thinking about a 2.0 to change some of the weird default values but if this was to require any breaking changes I'd like to fit them into the same release.

@dantman
Copy link
Contributor Author

dantman commented Jan 16, 2016

@jrit No, I've been making due with my workaround.

@jrit
Copy link
Collaborator

jrit commented Sep 8, 2016

WRI has a linkTransform and scriptTransform on it now, which should be able to handle transforming LESS files and other such things. Hope that is enough to close this without really missing anything.

@jrit jrit closed this as completed Sep 8, 2016
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

2 participants