Skip to content

Refactor: Access props from a single props argument #797

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mfisher87
Copy link
Member

@mfisher87 mfisher87 commented Jul 2, 2025

Description

I feel that while this is slightly more verbose, it reduces cognitive load when reading the component code. Previously, in order to know where a value comes from (i.e. props vs state), I needed to remember the names of the props. With this change, I don't need to know that because I can see the value is accessed from the props object.

This work was done by Claude, as it's a pretty rote and boring task not really worth spending human hours on. I haven't fully reviewed and applied my judgement, which is why this is still a draft. I'd like your input before I fully review and tweak this work for merge -- is this a change we agree we want?

@martinRenou @arjxn-py @gjmooney ? 🙏

Checklist

  • PR has a descriptive title and content.
  • PR description contains references to any issues the PR resolves, e.g. Resolves #XXX.
  • PR has one of the labels: documentation, bug, enhancement, feature, maintenance
  • Checks are passing.
    Failing lint checks can be resolved with:
    • pre-commit run --all-files
    • jlpm run lint

📚 Documentation preview: https://jupytergis--797.org.readthedocs.build/en/797/
💡 JupyterLite preview: https://jupytergis--797.org.readthedocs.build/en/797/lite

mfisher87 added 3 commits July 2, 2025 11:16
I feel that while this is more verbose, it reduces cognitive load when
reading the component code. Previously, in order to know where a value
comes from (i.e. props vs state), I needed to remember the names of all
the props. With this change, I don't need to know that because I can see
the value is accessed from the props object.
Copy link
Contributor

github-actions bot commented Jul 2, 2025

Binder 👈 Launch a Binder on branch mfisher87/jupytergis/props-as-arg

@mfisher87 mfisher87 changed the title Access props from a single props argument Refactor: Access props from a single props argument Jul 2, 2025
Copy link
Contributor

github-actions bot commented Jul 2, 2025

Integration tests report: appsharing.space

@gjmooney
Copy link
Collaborator

gjmooney commented Jul 3, 2025

I vote for destructuring props.

@mfisher87
Copy link
Member Author

mfisher87 commented Jul 3, 2025

Thanks for the input Greg! Can you provide the reasoning behind your vote?

The reason I felt this change was important is because I find destructuring is increasing the cognitive load of reading the code. I have to remember the names of props to know they are props. I feel readability is one of the most important attributes of a codebase, as code is written once and read many times. Cognitive load is one of the most important variables that contributes to readability, as it's a very limited resource (https://en.wikipedia.org/wiki/The_Magical_Number_Seven,_Plus_or_Minus_Two).

For example, reading the Annotation component, destructuring costs 4 of those slots. More realistically, I have to free those slots up to read the code, and frequently go back and check the function signature to refresh that information as I read :)

@mfisher87 mfisher87 mentioned this pull request Jul 22, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants