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(createElement): ignore prop value with null and undefined #198

Merged
merged 1 commit into from
Jun 19, 2022

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Jun 19, 2022

Please describe the changes this PR makes and why it should be merged:

Status

  • Code changes have been tested against prettier, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the codebase
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
    • This PR changes the internal workings with no modifications to the external API (bug fixes, performance improvements)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

I am using JSX with babel automatic runtime transformation and got Uncaught TypeError: setting getter-only property "children". It is caused by jsx function that set props.children to undefined:

props.children = undefined;

However, createElement will always set prop (including children) even if the prop is read-only and the prop value is undefined. The PR adds an additional null and undefined check before applying props. The corresponding test case has also been added as well.

@aidenybai
Copy link
Owner

Looks good, could you also make this change in useProps.ts as well?

@SukkaW
Copy link
Contributor Author

SukkaW commented Jun 19, 2022

Looks good, could you also make this change in useProps.ts as well?

I have checked useProps, it has already included a prop value check:

if (newPropValue) {
queueEffect(EffectTypes.SET_PROP, () => (el[propName] = newPropValue));
} else {

It should not be affected.

@aidenybai
Copy link
Owner

Cool, thanks for the help! Will be pushed out in the next patch

@aidenybai aidenybai merged commit 8e00f00 into aidenybai:main Jun 19, 2022
@aidenybai
Copy link
Owner

Hey! I seen you've made some PR's to the Million repo, If you'd be willing I'm extending an invitation for you to become a collaborator :)

Link: https://github.com/aidenybai/million/invitations

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.

None yet

2 participants