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

Less browser: append a new style tag instead of changing the content of existing tags #3199

Open
Nouzbe opened this issue Apr 17, 2018 · 5 comments

Comments

@Nouzbe
Copy link

Nouzbe commented Apr 17, 2018

Hi,

This is a feature request concerning less-browser.

Current behavior

There are currently 2 ways of compiling a less resource to css client-side: either put it in a <style type="text/less"> or href it in a <link type="text/less">. less will react to these two things.

Unfortunately it reacts in two different ways: for <link>, it downloads the resource, compiles it and adds it to a new <style> (see less-browser/index.js#L186). But for <style>, it compiles the content of the tag and replaces it in place (see less-browser/index.js#L68).

Description of the proposed feature

A single behavior would be nicer, and I believe that adding a new <style> tag is the best option (as opposed to changing tags that were put in the DOM by the application using less). With the current behavior indeed, using a <style type="text/less">, forces you to re-append it to the DOM each time that you perform a less.modifyVars.

You might say that if I want the less resource to stay there and be taken into account at each new modifyVars, I can just use a <link> tag. But it's not always handy. For instance, if you develop a library, using a <link> forces projects using you to maintain a custom webpack config, whereas using a <style> lets you bundle the resource yourself.

If changing the behavior is not possible, it could be enough to add a convention for it (for instance, if the style tag containing the less resource has an id, then it could remain untouched in the DOM and another style tag with the compiled css could be appended by less)

I hope that this is helpful. Anyway, thanks for the great tool!

@matthew-dean
Copy link
Member

Seems like a legit request. I suppose the rationale (I'm guessing) was that the existing style tag is not relevant. The fact that calling modifyVars appends a style tag multiple times is not ideal (if that's what's happening).

@seven-phases-max
Copy link
Member

seven-phases-max commented Apr 17, 2018

True. I also completely agree that modifyVars should reset any previous modifyVars calls (if we're interested to preserve something there it's times more efficient to do at the JS side of the corresponding code).
Though I doubt the current behaviour has anything to do with modifyVars as if I'm not a mistaken it was like that a long before any globalVars/modifyVars were introduced. I would search through the changelog and older issues to make sure there were no other reasons for it to work this way. But at first glance this looks more like a quirk/overlook/legacy thing rather than really intended/required behavior.

@matthew-dean
Copy link
Member

Though I doubt the current behaviour has anything to do with modifyVars as if I'm not a mistaken it was like that a long before any globalVars/modifyVars were introduced. I would search through the changelog and older issues to make sure there were no other reasons for it to work this way. But at first glance this looks more like a quirk/overlook/legacy thing rather than really intended/required behavior.

That would be my guess as well. That is, this behavior was not "designed" this way, it's just a legacy effect of a few features introduced over time.

@matthew-dean
Copy link
Member

Re-reading this issue, replacing a style tag is really the safest way to get a "fresh" set of styles. That is, if your modifyVars call causes as class to not be output, then simply adding a <style> tag without removing the former style tag will mean the class is still present.

Without a bunch of re-factoring, I don't know that any change there is really possible.

@Nouzbe
Copy link
Author

Nouzbe commented Jul 31, 2018

I can think of a way to fix this issue without too much refactoring:

We could rely on the presence of a target-style-id (for instance) attribute on the text/less style elements in order to link each of them to another style element that would hold its css output.

While loading the styles, if this attribute exists, we would use targetStyle = document.getElementById(targetStyleId) instead of style to output the css. Otherwise, we would generate an id, reference it in the target-style-id attribute of style and append the target style element to the DOM with this id.

If you dislike the idea of generating ids, then only doing the first part (checking for the existence of the attribute and the presence of the "target" style element with this id in the DOM) would already be enough to let users of less-browser get around this issue. It would only be up to them to provide the target style element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants