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] use a simpler insert and append function when not compile with hydratable #6525

Merged
merged 3 commits into from
Jul 21, 2021

Conversation

tanhauhau
Copy link
Member

Fixes #6462

Use back the old insert and append method, if it is not compile with hydratable: true

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

  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

if (compile_options.hydratable) {
if (internal_exports.has(`${name}_hydration`)) {
name += '_hydration';
} else if (internal_exports.has(`${name}Hydration`)) {
Copy link
Member

Choose a reason for hiding this comment

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

Question for understanding: What's this about? There's only methods ending with _hydration

Copy link
Member Author

@tanhauhau tanhauhau Jul 14, 2021

Choose a reason for hiding this comment

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

  1. the function you are looking at here, is the magic that turns
@insert(...)

into

import { insert } from 'svelte/internal';
insert(...);
  1. svelte/internal exposes a lot of methods, which includes insert and insert_dev, see https://github.com/sveltejs/svelte/blob/master/src/runtime/internal/dev.ts

the function you see here, decides on whether to use insert or insert_dev from @insert by checking the compileOptions.dev, similarly, we could decide to use insert or insert_hydration based on compileOptions.hydratable

  1. to determine the list of exports from svelte/internal, and then decide whether is there a xxx_dev exported, we build the svelte/internal and write them into a file internal_exports.ts, see

svelte/rollup.config.js

Lines 86 to 92 in 931738e

writeBundle(bundle) {
if (dir === 'internal') {
const mod = bundle['index.mjs'];
if (mod) {
fs.writeFileSync('src/compiler/compile/internal_exports.ts', `// This file is automatically generated\nexport default new Set(${JSON.stringify(mod.exports)});`);
}
}

for @xxx, the function above check whether xxx_hydration is present within internal_exports, if so, import xxx_hydration instead of xxx. it then checks if xxx_hydration_dev is present, if so it will use xxx_hydration_dev instead of xxx_hydration.

Copy link
Member

@dummdidumm dummdidumm Jul 14, 2021

Choose a reason for hiding this comment

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

Thanks for the thourough answer! What I don't understand though is why there's this inner if clause for each of these cases. The methods are called append_hydration_dev, but the if clauses look to me like there's also appendHydrationDev which is not the case, or am I misunderstanding something there?
Or in code:
Why this

								if (compile_options.hydratable) {
									if (internal_exports.has(`${name}_hydration`)) {
										name += '_hydration';
									} else if (internal_exports.has(`${name}Hydration`)) {
										name += 'Hydration';
									}
								}

and not this?

								if (compile_options.hydratable) {
									if (internal_exports.has(`${name}_hydration`)) {
										name += '_hydration';
									}
								}

Edit: aaah got the answer, it's the classes

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea.. there's SvelteComponentDev

export class SvelteComponentDev extends SvelteComponent {

and HtmlTagHydration

Copy link
Member

Choose a reason for hiding this comment

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

this seems like it could be worth a code comment

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.

Svelte "hello world" increased by 4.37kB (~59%) between Svelte 3.37.0 and 3.38.0
4 participants