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

perf: convert empty text nodes to comments #5280

Closed
wants to merge 1 commit into from

Conversation

shirotech
Copy link

@shirotech shirotech commented Aug 16, 2020

fixes #3586, closes #3642

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases, features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests with npm test or yarn test)

It's been almost a year since this PR has been opened #3642

Just wanted to reiterate on the current issue we have with text nodes as anchors, also updated some tests.

Performance

Comment nodes are considerably faster https://www.measurethat.net/Benchmarks/ShowResult/123052

Correctness

While text nodes are great for reducing clutter in the DOM inspector, it also influences and forces styles we don't need which can lead to unpredictable results. Comment nodes do not have this issue.

Real world example

On IE browsers support for Wordpress, if two text nodes are parallel to each other, the site would just crash. https://github.com/WordPress/WordPress/pull/461/files

Very keen to have this merged, so we don't have to keep syncing with our forks every time Svelte updates.

Not a breaking change

We have been running a fork for a while now without issues, also the minimal changes on existing tests suggests there should be no issues with having this merged.

Tagging in maintainers who have been involved in previous discussions @Rich-Harris @Conduitry @antony

Would be awesome if we can get a consensus on this, many thanks.

@shirotech
Copy link
Author

Dear sveltejs maintainers, any chance this can merged? Please let me know what else I can provide to make that happen, many thanks.

@antony
Copy link
Member

antony commented Nov 1, 2020

does this work in SSR mode? I've got very little context on how this works or what it does, but I notice the use of document and that doesn't exist server-side.

@pushkine
Copy link
Contributor

pushkine commented Nov 1, 2020

it also influences and forces styles we don't need which can lead to unpredictable results

Can you provide an example of that ?

Comment nodes are considerably faster

If you average all the tests together the results seem to be the same
https://www.measurethat.net/Benchmarks/ListResults/9220

[...] keep syncing with our forks every time Svelte updates

Is that really necessary ? Wouldn't it be easier to do through a bundler plugin ?

// rollup.config.js
import replace from 'rollup-plugin-replace';

export default {
	plugins: [
		replace({
			include: "node_modules/svelte/internal/*",
			"text('')": "document.createComment('')",
			delimiters: ["", ""],
		}),
		...
	],
	...
};

text nodes are great for reducing clutter in the DOM inspector

Real world example
On IE browsers [...]

Are there reasons for that change not to be exclusive to legacy mode ?
Or to at least avoid it in dev mode to address the cluttering ?

@shirotech
Copy link
Author

Is that really necessary ? Wouldn't it be easier to do through a bundler plugin ?

// rollup.config.js
import replace from 'rollup-plugin-replace';

export default {
	plugins: [
		replace({
			include: "node_modules/svelte/internal/*",
			"text('')": "document.createComment('')",
			delimiters: ["", ""],
		}),
		...
	],
	...
};

Very nice! didn't know you can do that. Works much better, guess will be closing this then.

@shirotech shirotech closed this Nov 13, 2020
@shirotech shirotech deleted the text-comment-node branch November 13, 2020 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A lot of empty text nodes - I mean A LOT - performance issue
4 participants