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

WiP: Inline scope components #2791

Closed
wants to merge 3 commits into from
Closed

WiP: Inline scope components #2791

wants to merge 3 commits into from

Conversation

barneycarroll
Copy link
Member

Description

Add a new clause in the updateNode render loop which determines scope components to be equivalent if their source is identical.

Motivation and Context

The ability to declare a component inline allows for ad-hoc state capture without the rigmarole of breaking out to define a component elsewhere, determine what contextual reference access it needs, write the attrs interface accordingly, and then invoke it at call-site. In fact it facilitates iterative component development by enabling authors to experiment with component logic while benefiting from full call-site context reference access – leaving restrictive interface design as a formal 'ejection' exercise rather than a constant site of friction.

The internal logic determining equivalence relies on oldTag.toString() === tag.toString. Object and class components do not have as straightforward source-sniffing APIs, and the idiomatic style of OOP component-based application design means there is less benefit to the inter-scope access inline components provide.

How Has This Been Tested?

I've written a new test module but – strangely – these only pass if I run it alone, ie npm run test:js ./render/test-inlineComponent.js; maybe it's something to do with my slap-dash global Mithril instantiation at the top of the test file?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/changelog.md

@barneycarroll barneycarroll added Type: Meta/Feedback For high-level discussion around the project and/or community itself Area: Core For anything dealing with Mithril core itself Action trigger Not intended for merge; the PR is created purely to trigger Github actions. labels Jun 25, 2022
@barneycarroll barneycarroll requested a review from pygy June 25, 2022 17:17
@dead-claudia dead-claudia requested a review from a team as a code owner September 23, 2024 11:58
Copy link
Collaborator

@JAForbes JAForbes left a comment

Choose a reason for hiding this comment

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

Bit of a stretch, but if you had two components from different modules with the same body source code that access different closured scope state wouldn't the else if branch in render treat them as the same component potentially because they have the same function::toString(). That could cause some potential issues as the component would have the incorrect closure state.

It would be nice to be able to solve that problem, but also I really like this idea.

@dead-claudia dead-claudia deleted the branch next September 25, 2024 05:23
@dead-claudia
Copy link
Member

Closed due to the next branch being deleted. Thanks to https://github.com/orgs/community/discussions/139697, I can't resurrect this PR, so it'd have to be re-created against main. I'll leave the branch around for now.

I'm a bit -1 on the present design, though, due to the closure state concerns James cited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action trigger Not intended for merge; the PR is created purely to trigger Github actions. Area: Core For anything dealing with Mithril core itself Type: Meta/Feedback For high-level discussion around the project and/or community itself
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants