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

Expand types for insert after/before #802

Merged
merged 2 commits into from
Jul 14, 2020

Conversation

maier49
Copy link
Contributor

@maier49 maier49 commented Jun 11, 2020

Type: bug

The following has been addressed in the PR:

Description:
This expands the types for insertBefore/insertAfter. This would I believe be a backwards compatible fix but I think may be the appropriate fix in general.
We currently allow inserting before/after at the root and when the parent is not wrapped, and requiring a parent would break both of those use cases. Making the parent argument optional could allow the user to opt into typing but seems klugey since the parent isn't actually needed.

Additionally the only concern with widening the type is that we'd allow the parent's child type to be violated, but even if we enforced it here the replace function accepts any DNode. It seems inconsistent if our API enforces the types of siblings but not the node itself.
Resolves #801

@maier49 maier49 requested a review from agubler June 11, 2020 22:47
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 11, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 50975a3:

Sandbox Source
peaceful-swirles-6b0jp Configuration
clever-cherry-2rxxk Issue #801
dreamy-banach-gi775 Issue #801

@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #802 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #802   +/-   ##
=======================================
  Coverage   97.52%   97.52%           
=======================================
  Files         126      126           
  Lines        7927     7927           
  Branches     1824     1824           
=======================================
  Hits         7731     7731           
  Misses        196      196           
Impacted Files Coverage Δ
src/testing/renderer.ts 97.58% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c70b240...50975a3. Read the comment docs.

@agubler
Copy link
Member

agubler commented Jul 8, 2020

@maier49 I think this is backwards compatible and fixes the ability to insert siblings - at the moment it types children to the wrapped siblings child type which isn’t correct at all. The fix would allow the children type of the parent to be violated like you mention though.

I think the correct fix for the next major release would be to require the parent on the API to be able to correctly enforce the child type when working with siblings.

@maier49 maier49 merged commit e9faf2c into dojo:master Jul 14, 2020
@maier49 maier49 deleted the fix-child-types-in-templates branch July 14, 2020 02:16
agubler pushed a commit to agubler/framework that referenced this pull request Jul 22, 2020
Change insertBefore/After to allow any template children rather than typing based on the targeted node.
@agubler agubler mentioned this pull request Jul 22, 2020
3 tasks
agubler added a commit that referenced this pull request Jul 22, 2020
* Fix types for insert after/before (#802)

Change insertBefore/After to allow any template children rather than typing based on the targeted node.

* Fix: Rendering for multiple nested `head` and `body` nodes (#812)

* failing unit test for nested body and head tags

* skip head or body node when finding insert before node

* move JSX types to tsx namespace (#818)

* fix unit test that was missed on master

Co-authored-by: Bradley Maier <bmaier@sitepen.com>
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.

Incorrect type for children parameter of .insertBefore (and variants)
2 participants