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

fix: allow embedding two pages generated into the same page in "embedded" mode #9610

Merged
merged 20 commits into from
Apr 17, 2023

Conversation

ejmurra
Copy link
Contributor

@ejmurra ejmurra commented Apr 6, 2023

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This PR namespaces the global object that initializes the root component in embedded mode.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

This is an attempt to fix #9576

I wasn't sure how to write tests for this but will happily add some with some guidance.

@changeset-bot
Copy link

changeset-bot bot commented Apr 6, 2023

🦋 Changeset detected

Latest commit: 72d2636

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benmccann
Copy link
Member

@ejmurra a very small note - don't put a space before the "x" when checking the boxes in the PR description. GitHub will then render them as check boxes

@benmccann
Copy link
Member

Can you clarify what this is fixing? Why would the timestamp not be sufficient? Or why don't you just give the apps different names? How can you be sure the two apps have different leaf nodes?

@ejmurra
Copy link
Contributor Author

ejmurra commented Apr 6, 2023

It's not two apps. It's a single app that renders two pages and tries to inline them into a separate 3rd page. Repro steps here: #9576

I'm no svelte expert, but my understanding after spending time to create this PR is that the leaf nodes are going to be unique in the same app and that there will be one for each page. Please correct me if I'm wrong about that!

@benmccann
Copy link
Member

Ah, sorry I missed the link to the issue. Usually people put that above the boilerplate. If you do that next time hopefully it will keep it from getting overlooked

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

you'll need to run pnpm format to fix the failing lint job

.changeset/late-toes-cry.md Outdated Show resolved Hide resolved
@ejmurra
Copy link
Contributor Author

ejmurra commented Apr 11, 2023

I've added an integration test for this in test/apps/options-3/. The test could potentially be moved into another test app but it requires config embedded: true and no csp directives.

@benmccann
Copy link
Member

We try to avoid adding new test projects and just reuse existing ones. Perhaps that won't be possible here with the use of the embedded option though. I don't fully know if this is worth adding a test for, but if we do want to then we should make it more like a default project. It looks like you copied another test app that was testing all sorts of options and so there's a bunch of really uncommon stuff in your test project that will make things harder to understand like calling the source directory source instead of src and calling the Vite config vite.custom.config, etc.

@benmccann benmccann changed the title fix: added a leaf node hash to global namespace to prevent clobbering in embedded mode fix: allow embedding two pages generated into the same page in "embedded" mode Apr 17, 2023
@Rich-Harris
Copy link
Member

baffled by these test failures

@Rich-Harris Rich-Harris merged commit 367067f into sveltejs:master Apr 17, 2023
@Rich-Harris
Copy link
Member

thank you!

This was referenced Apr 17, 2023
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.

sveltekit nodes are clobbering each other in "embedded" mode
3 participants