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

Custom style tag #5639

Closed
wants to merge 39 commits into from
Closed

Custom style tag #5639

wants to merge 39 commits into from

Conversation

ivanhofer
Copy link
Contributor

@ivanhofer ivanhofer commented Nov 3, 2020

issue: #3940

I added an option to allow to specify a Element where the styles should be applied. Currently the styles are always appended to document.head.
This PR is useful for a svelte-application that is rendered inside a shadow-dom. Because of the style-encapsulation that come with shadow-doms, the application will not have any styles when rendered inside a shadow-dom.
Please let me know if you have any concern with this approach?

@ivanhofer
Copy link
Contributor Author

ivanhofer commented Nov 4, 2020

I'm not sure why some tests are failing.

I added a new parameter to the init-function in src/runtime/internal/Component.ts.
I updated all references I could find to also use the new parameter. But still some tests (test\runtime\samples\bitmask-overflow-slot*) are failing, because the new parameter is not applied. I would need some help to figure out what the problem is.

Another test that is failing is: js/samples/collapses-text-around-comments. I find it is a bad test, because it compares the generated output 1:1. The test will always fail if the code would look slightly different. It does not test any functionality.
As I can see currently only tests like these fail.

Can someone please help?

@ivanhofer
Copy link
Contributor Author

@benmccann I now added the noop-parameter to all tests.
Currently only 1 test fails because of some timeout issue. I looked at the generated output and I could not find why my code-changes should affect this test. Could you please provide me some help?

@tanhauhau
Copy link
Member

@ivanhofer thank you for making the change. Usually we're more reluctant to merge a new feature if it increases the output size for the base case.

is it possible to support this without adding extra code for users that does not need this feature?

@ivanhofer
Copy link
Contributor Author

ivanhofer commented Nov 11, 2020

Hi @tanhauhau,
I am aware that my changes slightly increase the size of the compiled output.

For projects, that use the compiler option emitCss: false, the generated output will be less than before, because I refactored the add_css method. In my project with 72 components I could get the output from 269.713 bytes to 265.945 bytes (- 3768 bytes).

For projects that use emitCss: true this changes would mean a slightly increase in the generated output. When the output is minified currently the changes would mean additional 83 bytes for the changes in the init function and additional ~2 bytes per component. In my project with 72 components the output slightly increased from 263.129 bytes to 263.371 bytes (+ 242 bytes) when using emitCss: true with the changes of this PR.

I will check if there is a possibility to decrease the generated output for projects using emitCss: true, but I think there is no possibility to make these changes without adding a few bytes to the output.

@ivanhofer
Copy link
Contributor Author

@tanhauhau I just managed to decrease the amount of added code to 8 byte.
The new size of the generated output of my project with 72 components is:

emitCss: false
before:  269.271 bytes
after:   266.541 bytes 
change:  - 2.730 bytes
emitCss: true
before:  263.191 bytes 
after:   263.199 bytes
change:      + 8 bytes

Are these changes good for you? What do you think?

PS: I will later revert my changes to the tests, to make them work again.

@ivanhofer
Copy link
Contributor Author

@tanhauhau my final analysis on bundle size using the official starter template without changes is:

emitCss: true
before:  3.284 bytes
after:   3.292 bytes 
change:    + 8 bytes
emitCss: false
before:  3.646 bytes
after:   3.787 bytes 
change:  + 141 bytes

When using emitCss: true the output will always be 8 bytes more, no matter how many components are used.

When using emitCss: false the output for a single component is 141 bytes more.
But the output is less when using more components:

1 component:
before:  3.646 bytes
after:   3.787 bytes
change:  + 141 bytes

2 components:
before:  4.854 bytes
after:   4.940 bytes
change:   + 86 bytes

3 components:
before:  5.902 bytes
after:   5.927 bytes
change:   + 25 bytes

4 components:
before:  6.938 bytes
after:   6.902 bytes
change:   - 36 bytes

5 components:
before:  7.974 bytes
after:   7.883 bytes
change:   - 91 bytes

I cloned the App.svelte component from the starter template and made a App2.svelte component and added it as a child of App.svelte and so on...

As you can see the output with the changes in this branch become less than the output from the master-branch when using 4 components or more.

I hope that the size-savings by using multiple components justifies the additional size for a single component.

@ivanhofer
Copy link
Contributor Author

I managed to strip another few bytes from the output size:

1 component:
before:  3.646 bytes
after:   3.790 bytes
change:  + 144 bytes

2 components:
before:  4.854 bytes
after:   4.921 bytes
change:   + 65 bytes

3 components:
before:  5.902 bytes
after:   5.880 bytes
change:   - 22 bytes

4 components:
before:  6.938 bytes
after:   6.827 bytes
change:  - 111 bytes

5 components:
before:  7.974 bytes
after:   7.780 bytes
change:  - 194 bytes

As you can see, now the output for a bundle with 3 components is smaller than before.

I now reduced the output of the add_css function of each component to a minimum. I moved the "svelte-" and "-style" parts from the id to the shared method. At first glance it looks a bit odd, but reduces the bundle size by a few more bytes.
What do you guys think?

export function init(component, options, instance, create_fragment, not_equal, props, dirty = [-1]) {
const parent_component = current_component;
set_current_component(component);

const prop_values = options.props || {};

const $$: T$$ = component.$$ = {
...component.$$,
Copy link
Member

Choose a reason for hiding this comment

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

i understand that doing this was to make the code size for the base case unchanged.
but i'm ambivalent of doing this.

what do you think @Conduitry

Copy link
Contributor Author

@ivanhofer ivanhofer Nov 18, 2020

Choose a reason for hiding this comment

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

I think the only option without additional code for the base output is to rewrite the function add_css_to_component to this:

export function add_css_to_component(component, add_css, options) {
	component.customStyleTag = options.customStyleTag || current_component && current_component.customStyleTag;
	add_css(component.customStyleTag || document.head);
}

But then it is a "hidden" property on the class itself.
I chose the option to include it in the $$ context object because that's why the context object exists.

Copy link
Contributor Author

@ivanhofer ivanhofer Nov 19, 2020

Choose a reason for hiding this comment

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

another solition could be to save the information in a variable like the current_component:

let appendStylesTo = document.head

export function add_css_to_component(add_css, options) {
   if (options.customStyleTag) appendStylesTo = options.customStyleTag
   add_css(appendStylesTo);
}

@ivanhofer
Copy link
Contributor Author

@tanhauhau I now removed customStyleTag from the $$ object in the component. With these changes the output size is not affected when using emitCss: true.

When using emitCss, with this changes, the output size is like this:

1 component:
before:  3.646 bytes
after:   3.680 bytes
change:   + 34 bytes

2 components:
before:  4.854 bytes
after:   4.770 bytes
change:   - 84 bytes

The output size with this PR applied reduces by ~100bytes for each component comparing the current implementation on master.

I was looking into writing tests for customStyleTag implementation, but I could't find a good solution looking at the existing tests. Has someone an idea, how I could write the tests?

@ivanhofer ivanhofer mentioned this pull request Nov 29, 2020
4 tasks
@ivanhofer
Copy link
Contributor Author

ivanhofer commented Jan 8, 2021

closing in favor of #5870, which is a clean re-implementation of this PR

@ivanhofer ivanhofer closed this Jan 8, 2021
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.

3 participants