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: key in p could fail if p is null #11388

Merged
merged 9 commits into from
May 10, 2024
Merged

fix: key in p could fail if p is null #11388

merged 9 commits into from
May 10, 2024

Conversation

fsoft72
Copy link
Contributor

@fsoft72 fsoft72 commented Apr 30, 2024

I found a glitch with a complex input component while testing next.118. It worked fine in next.115 – maybe something changed in the updates? Hope this helps!

I run all tests before submitting.

Copy link

changeset-bot bot commented Apr 30, 2024

⚠️ No Changeset found

Latest commit: 0b099b2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@dummdidumm
Copy link
Member

Can you please add a test that fails without the fix?

@fsoft72
Copy link
Contributor Author

fsoft72 commented Apr 30, 2024

Hello @dummdidumm

Yes, I am more than happy to provide a failing test, but to reproduce it I need to add a lot context (I can rework my component to remove all external dependencies, but it'll be big). Also, it is an issue that shows up interactively, while I have seen the tests directory contains automatic tests. Could you please point me to some documentation I can read to create the perfect test?

@Rich-Harris
Copy link
Member

Let's start with a repro and go from there

@fsoft72
Copy link
Contributor Author

fsoft72 commented May 1, 2024

Ok, so I have created the smallest Svelte Kit project possible.
As a side note: it does not work with next.120 either.

You can find the project here:

svelte5test.tar.gz

Steps to reproduce:

  1. tar xvf svelte5test.tar.gz
  2. cd svelte5test
  3. pnpm i
  4. pnpm dev
  5. open the browser to the given location

As soon as you open the browser, you should get the error in the browser debugger console.

I am pasting here the relevant messages:

chunk-N5QL6BBT.js?v=2b4b5bdf:2491 Uncaught TypeError: Cannot use 'in' operator to search for 'Symbol($state)' in undefined
    at Object.has (chunk-N5QL6BBT.js?v=2b4b5bdf:2491:15)
    at Object.has (chunk-N5QL6BBT.js?v=2b4b5bdf:2457:16)
    at Object.has (chunk-N5QL6BBT.js?v=2b4b5bdf:2457:16)
    at Module.deep_read_state (chunk-PY2A4NTW.js?v=2b4b5bdf:1604:20)
    at Input.svelte:59:15
    at Object.fn (chunk-PY2A4NTW.js?v=2b4b5bdf:416:5)
    at execute_reaction_fn (chunk-PY2A4NTW.js?v=2b4b5bdf:1052:22)
    at execute_effect (chunk-PY2A4NTW.js?v=2b4b5bdf:1169:20)
    at create_effect (chunk-PY2A4NTW.js?v=2b4b5bdf:368:7)
    at render_effect (chunk-PY2A4NTW.js?v=2b4b5bdf:443:10)
has @ chunk-N5QL6BBT.js?v=2b4b5bdf:2491
has @ chunk-N5QL6BBT.js?v=2b4b5bdf:2457
has @ chunk-N5QL6BBT.js?v=2b4b5bdf:2457
deep_read_state @ chunk-PY2A4NTW.js?v=2b4b5bdf:1604
(anonymous) @ Input.svelte:59
(anonymous) @ chunk-PY2A4NTW.js?v=2b4b5bdf:416
execute_reaction_fn @ chunk-PY2A4NTW.js?v=2b4b5bdf:1052
execute_effect @ chunk-PY2A4NTW.js?v=2b4b5bdf:1169
create_effect @ chunk-PY2A4NTW.js?v=2b4b5bdf:368
render_effect @ chunk-PY2A4NTW.js?v=2b4b5bdf:443
legacy_pre_effect @ chunk-PY2A4NTW.js?v=2b4b5bdf:415
Input @ Input.svelte:59
(anonymous) @ chunk-N5QL6BBT.js?v=2b4b5bdf:135
execute_reaction_fn @ chunk-PY2A4NTW.js?v=2b4b5bdf:1052
execute_effect @ chunk-PY2A4NTW.js?v=2b4b5bdf:1169
create_effect @ chunk-PY2A4NTW.js?v=2b4b5bdf:368
branch @ chunk-PY2A4NTW.js?v=2b4b5bdf:449
(anonymous) @ chunk-N5QL6BBT.js?v=2b4b5bdf:131
execute_reaction_fn @ chunk-PY2A4NTW.js?v=2b4b5bdf:1052
execute_effect @ chunk-PY2A4NTW.js?v=2b4b5bdf:1169
create_effect @ chunk-PY2A4NTW.js?v=2b4b5bdf:368
block @ chunk-PY2A4NTW.js?v=2b4b5bdf:446
(anonymous) @ chunk-N5QL6BBT.js?v=2b4b5bdf:124
(anonymous) @ FormCreator.svelte:156

Hope it helps.

I am always here to give you more feedback.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Minimum reproducible - could you add this as a test to the runtime-legacy test suite? Thank you!

packages/svelte/src/internal/client/reactivity/props.js Outdated Show resolved Hide resolved
fsoft72 and others added 6 commits May 2, 2024 13:06
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
This commit adds a new Child component and its props to the prop-p-is-null sample in the Svelte package. The Child component receives props using the spread operator and the props are passed as undefined. This change ensures that the Child component is properly rendered with the correct props.

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
@fsoft72
Copy link
Contributor Author

fsoft72 commented May 8, 2024

Minimum reproducible - could you add this as a test to the runtime-legacy test suite? Thank you!

Hello @dummdidumm, I have added the minimum reproducible test in the runtime-legacy test suite.

Sorry it did take so long, I overlooked your previous message.

@dummdidumm dummdidumm merged commit f219c79 into sveltejs:main May 10, 2024
6 of 8 checks passed
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