Skip to content

refactor: inline App getters and setters#2904

Closed
timreichen wants to merge 1 commit into
freshframework:mainfrom
timreichen:inline-app-getters-and-setters
Closed

refactor: inline App getters and setters#2904
timreichen wants to merge 1 commit into
freshframework:mainfrom
timreichen:inline-app-getters-and-setters

Conversation

@timreichen

Copy link
Copy Markdown
Contributor

No description provided.

@marvinhagemeister marvinhagemeister left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Inlining these means exposing them as part of the public API. I'm not sure if the router class or the island registry is finalized enough to expose as part of the public API.

@timreichen

Copy link
Copy Markdown
Contributor Author

Inlining these means exposing them as part of the public API. I'm not sure if the router class or the island registry is finalized enough to expose as part of the public API.

How about accessing it via private symbol?

@marvinhagemeister

Copy link
Copy Markdown
Contributor

Inlining these means exposing them as part of the public API. I'm not sure if the router class or the island registry is finalized enough to expose as part of the public API.

How about accessing it via private symbol?

I'd reject such refactorings as they it only comes down to personal taste which way to prefer. I'd rather spend the available energy elsewhere, where user's can benefit from the changes rather than changing coding styles.

@marvinhagemeister

Copy link
Copy Markdown
Contributor

Closing as we're not going to merge this.

@timreichen

Copy link
Copy Markdown
Contributor Author

Inlining these means exposing them as part of the public API. I'm not sure if the router class or the island registry is finalized enough to expose as part of the public API.

How about accessing it via private symbol?

I'd reject such refactorings as they it only comes down to personal taste which way to prefer. I'd rather spend the available energy elsewhere, where user's can benefit from the changes rather than changing coding styles.

I disagree that this is only personal taste. Although functionally it might work the same, it is not obvious (at least it was not for me) why there are exported variables that are not defined until you read the rest of the code to find where they are assigned and what they do. This wastes time and makes it harder to contribute for new contributors.

@iuioiua

iuioiua commented May 13, 2025

Copy link
Copy Markdown
Contributor

I disagree that this is only personal taste. Although functionally it might work the same, it is not obvious (at least it was not for me) why there are exported variables that are not defined until you read the rest of the code to find where they are assigned and what they do. This wastes time and makes it harder to contribute for new contributors.

I agree

@marvinhagemeister

marvinhagemeister commented May 13, 2025

Copy link
Copy Markdown
Contributor

@iuioiua

iuioiua commented May 13, 2025

Copy link
Copy Markdown
Contributor

I think the need to read that page might be more to our point that this way of doing things might not be so clear to the reader up front. I like Tim's change because it's immediately clear to the reader what's going on and a lot more beginner friendly. The main point against this change, however, is that it would make the router, island registry and build cache properties public. And it's good to keep the number of symbols in the public API to a minimum.

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