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: allow to pass options to insert function through style.use() #535

Merged
merged 3 commits into from Sep 14, 2021

Conversation

OlegWock
Copy link
Contributor

@OlegWock OlegWock commented Sep 12, 2021

Allow to pass additional parameter to style.use(anything) which will be passed to insert function. This allows to insert each tag in different places and especially useful for Shadow DOM

Resolves #328

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

I'm using style-loader in WebExtension and we heavily rely on Shadow DOM (we inject our widgets into 3rd party sites and want them look always nice, same on all sites). But it was a pain to inject styles into Shadow DOM (you better not peek on my old webpack config haha!). This small change allows to pass custom options (any object basically) to the styles.use call, which later will be passed to insert function. This allows customizing insert logic: now you can insert each style into different place and do whatever you want for each style individually

Breaking Changes

This shouldn't break anything

Additional Info

Resolves #328 (not in the proposed way tho)

Allow to pass additional parameter to `style.use(anything)` which will be passed to `insert` function. This allows to insert each tag in different places and especially useful for Shadow DOM

Resolves webpack-contrib#328
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 12, 2021

CLA Signed

The committers are authorized under a signed CLA.

@OlegWock OlegWock marked this pull request as ready for review September 12, 2021 16:34
@alexander-akait
Copy link
Member

Good idea, anyway can you provide examples:

I'm using style-loader in WebExtension and we heavily rely on Shadow DOM (we inject our widgets into 3rd party sites and want them look always nice, same on all sites). But it was a pain to inject styles into Shadow DOM (you better not peek on my old webpack config haha!). This small change allows to pass custom options (any object basically) to the styles.use call, which later will be passed to insert function. This allows customizing insert logic: now you can insert each style into different place and do whatever you want for each style individually

I want to add more tests and examples here?

@OlegWock
Copy link
Contributor Author

OlegWock commented Sep 13, 2021

@alexander-akait previously we injected style tag to all shadow doms present on page because there was no way to specify which component loaded styles:

component.js

import css from './preferences.less';
import ReactDOM from "react-dom";
import React from "react";

class MyComponent extends React.Component {}

const attachMyComponentToPage = ($target, props) => {
    if (!$target) {
        $target = $(`<div id="MyComponentShadow" data-ad-view="true"></div>`);
        $('body').append($target);
    }
    let shadow = $target[0].attachShadow({mode: 'open'});

    ReactDOM.render(<MyComponent {...props} />, shadow, () => {
        css.use();
    });
};

Settings for styleLoader

{
        injectType: 'lazyStyleTag',
        insert: function insert(el) {
            let id = '_' + Math.random().toString(36).substr(2, 9);
            console.log('Style loader insert:', el, id);
            el.setAttribute('data-ad-id', id);
            let shadows = document.querySelectorAll('[data-ad-view]');
            shadows.forEach(function (node) {
                if (node.shadowRoot) {
                    node.shadowRoot.appendChild(el.cloneNode(true));
                } else {
                    node.appendChild(el.cloneNode(true));
                }
            });
            if (shadows.length === 0) {
                document.querySelector('head').appendChild(el.cloneNode(true));
            }
        },
        styleTagTransform: function (css, style) {
            let id = style.getAttribute('data-ad-id');
            let styleNodes = [];

            let shadows = document.querySelectorAll('[data-ad-view]');
            shadows.forEach(function (node) {
                console.log('Handling node', node);
                if (node.shadowRoot) {
                    console.log('Found shadow, appending child');

                    let styleNode = node.shadowRoot.querySelector(`style[data-ad-id="${id}"]`);
                    if (styleNode) styleNodes.push(styleNode);
                } else {
                    console.log('Ordinary node, appending child');

                    let styleNode = node.querySelector(`style[data-ad-id="${id}"]`);
                    if (styleNode) styleNodes.push(styleNode);
                }
            });
            if (shadows.length === 0) {
                console.log('No data-ad-view elements found, appending to page head');
                let styleNode = document.querySelector(`style[data-ad-id="${id}"]`);
                if (styleNode) styleNodes.push(styleNode);
            }

            for (let styleNode of styleNodes) {
                if (styleNode.styleSheet) {
                    styleNode.styleSheet.cssText = css;
                } else {
                    while (styleNode.firstChild) {
                        styleNode.removeChild(styleNode.firstChild);
                    }
                    styleNode.appendChild(document.createTextNode(css));
                }
            }
        },
    };

Let me explain what all this mess does. I assume this isn't perfect solutions and there is way to do it better with current version (without proposed changes to .use method). Here, in insert function we assign random ID to the style tag and walk through all [data-ad-view] elements (it's our shadow dom containers) and append copy of <style> into each shadow. Later, in styleTagTransform we retrieve this ID to find all <style> tags we created on previous stage and populate them with css string.

Because we often display multiple widgets on same page and all widgets use multiple .css files (one for antd, one for fontawesome, one for custom styles) we end up with a bunch of <style> tags in each shadow DOM (in addition to out hacky webpack config), while only small part of them are used in current Shadow DOM.

With changes proposed in this PR you can pass parameters to .use which will be later passed to insert function. I have such use case in mind:

component.js

import css from './preferences.less';
import ReactDOM from "react-dom";
import React from "react";

class MyComponent extends React.Component {}

const attachMyComponentToPage = ($target, props) => {
    if (!$target) {
        $target = $(`<div id="MyComponentShadow" data-ad-view="true"></div>`);
        $('body').append($target);
    }
    let shadow = $target[0].attachShadow({mode: 'open'});

    ReactDOM.render(<MyComponent {...props} />, shadow, () => {
        css.use({target: shadow}); // Note we're now passing shadow root to .use call
    });
};

Settings for styleLoader

{
        injectType: 'lazyStyleTag',
        insert: function insert(el, options) {
            (options.target || document.querySelector('head')).appendChild(el);
        }
    };

I think there might be other use cases (maybe you need to know which component invoked particular styles) but this^^ is main one I had in mind.


While writing this comment I thought it might be useful to pass options from .use call to styleTagTransform too. But I'd like to hear your opinion on all this

@alexander-akait
Copy link
Member

While writing this comment I thought it might be useful to pass options from .use call to styleTagTransform too. But I'd like to hear your opinion on all this

sounds good too

Note - maybe help in future webpack-contrib/css-loader#1368

@OlegWock
Copy link
Contributor Author

it might be useful to pass options from .use call to styleTagTransform too

Done. Also changed

options.insertOptions = insertOptions || {};

to just

options.insertOptions = insertOptions;

in case someone would like to pass false or null here for whatever reason

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Let's add test for unuse to sure we remove the same target

@OlegWock
Copy link
Contributor Author

If I understand correctly, insert receives pre-made <style> tag. Reference to it stored internally and used to remove that tag (wherever it is). I.e. changes from this PR doesn't affect .unuse() in any way

@alexander-akait
Copy link
Member

Yep, just need to add test(s) to avoid break it/regression in future

@OlegWock
Copy link
Contributor Author

I'm not very proficient with Jest, I hope these tests are okay

@alexander-akait
Copy link
Member

Okay, I will finish tests after merge it

@alexander-akait
Copy link
Member

I will finish this PR

@alexander-akait alexander-akait merged commit f8ef63b into webpack-contrib:master Sep 14, 2021
@OlegWock
Copy link
Contributor Author

@alexander-akait Thanks! Do you have any plans when to release new version (to npm) with these changes?

@alexander-akait
Copy link
Member

@OlegWock today/tomorrow, want to finish some features for css-loader, because it requires more logic here

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.

Allow importing code to determine where styles are inserted
2 participants