Skip to content

Update project structure #183

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

Merged
merged 14 commits into from
Dec 2, 2022
Merged

Update project structure #183

merged 14 commits into from
Dec 2, 2022

Conversation

jamesrweb
Copy link
Member

@jamesrweb jamesrweb commented Sep 7, 2022

Proposed Changes

  • Update project folder structure to cleanup index.tsx

Additional Notes (optional)

This is a proposal for a new structure within the src directory. Happy for feedback or change suggestions as always 😄!

Please ensure exports remain exactly the same in the new index.tsx as those within the current index.tsx.

@jamesrweb jamesrweb requested a review from a team September 7, 2022 09:04
@jamesrweb jamesrweb self-assigned this Sep 7, 2022
@jamesrweb jamesrweb requested review from yevdyko and removed request for a team September 7, 2022 09:04
@jamesrweb jamesrweb force-pushed the update-project-structure branch from 91a90d7 to 0605c2d Compare September 7, 2022 09:09
@jamesrweb jamesrweb enabled auto-merge (squash) September 7, 2022 09:10
@jamesrweb
Copy link
Member Author

@yevdyko have you had a chance to look at this one yet? Would be good for the v4 release I think but let me know your feedback so we can iterate if necessary 👍🏻

Copy link
Contributor

@yevdyko yevdyko left a comment

Choose a reason for hiding this comment

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

@jamesrweb Thanks for bringing this up!

I'm not sure this is a good idea for a 120 line library. After such a fragmented structure, it became harder to understand how things work, having to remember what happens in each file instead of evaluating everything in one glance on one screen. I agree that this granularity is good for large projects or with the potential for rapid expansion, which I don't think is the case for this component. So I wouldn't want that change when we have a single line import, each function in separate files, I don't see the benefits of that. I understand that this is a matter of taste, so I don't think my opinion is decisive here. What do you find advantages to the new structure other than defragmenting into separate files?

@jamesrweb
Copy link
Member Author

@jamesrweb Thanks for bringing this up!

I'm not sure this is a good idea for a 120 line library. After such a fragmented structure, it became harder to understand how things work, having to remember what happens in each file instead of evaluating everything in one glance on one screen. I agree that this granularity is good for large projects or with the potential for rapid expansion, which I don't think is the case for this component. So I wouldn't want that change when we have a single line import, each function in separate files, I don't see the benefits of that. I understand that this is a matter of taste, so I don't think my opinion is decisive here. What do you find advantages to the new structure other than defragmenting into separate files?

I think the main benefit of doing it this way is being more explicit about the borders and using explicit import export statements for domain specific areas like the contracts for example. Mixing it all together is nice in one sense but it can also be confusing when you have to scroll up and down all the time. I am easy either way also but I feel the downsides are balanced with the ups and we can always reorganise later if necessary.

@yevdyko
Copy link
Contributor

yevdyko commented Sep 14, 2022

Okay, how about putting all the contracts in one file named contracts.ts, for example. And place the main component ReactP5Wrapper in the file index.ts and not in a separate one, since the main logic is there. How about that?

@jamesrweb
Copy link
Member Author

@jamesrweb Thanks for bringing this up!
I'm not sure this is a good idea for a 120 line library. After such a fragmented structure, it became harder to understand how things work, having to remember what happens in each file instead of evaluating everything in one glance on one screen. I agree that this granularity is good for large projects or with the potential for rapid expansion, which I don't think is the case for this component. So I wouldn't want that change when we have a single line import, each function in separate files, I don't see the benefits of that. I understand that this is a matter of taste, so I don't think my opinion is decisive here. What do you find advantages to the new structure other than defragmenting into separate files?

I think the main benefit of doing it this way is being more explicit about the borders and using explicit import export statements for domain specific areas like the contracts for example. Mixing it all together is nice in one sense but it can also be confusing when you have to scroll up and down all the time. I am easy either way also but I feel the downsides are balanced with the ups and we can always reorganise later if necessary.

Okay, how about putting all the contracts in one file named contracts.ts, for example. And place the main component ReactP5Wrapper in the file index.ts and not in a separate one, since the main logic is there. How about that?

Contracts by definition should be separated though IMO. I also like the components being seperated from one another. I guess for me it is either do it this way or leave it as things are. I kind of like this way though but happy to not do it if theres a good reason. I don't know, personally I think it's fine how it is done here after re-reviewing it.

Maybe we move the Memo from index.ts into a component though and just export that like the other exports then, what do you think of that as an idea for example?

Lets keep iterating until we get it right 🚀

@yevdyko
Copy link
Contributor

yevdyko commented Sep 14, 2022

I don't think these one-line files make sense to me:

image

It's like using design patterns for the sake of patterns themselves, it seems to me we can spread everything over several files, but such granularity as we have in the current PR has no advantages.

@yevdyko
Copy link
Contributor

yevdyko commented Sep 14, 2022

Why not have the main component in the index if the entire library consists of that component?

@jamesrweb
Copy link
Member Author

I don't know, I think that there are no disadvantages either and concerns are now cleanly separated in some form from one another. I also don't think it makes sense to abstract one component but not the other. You know what I mean? Consistency in whatever form is better than partial adherence.

@yevdyko
Copy link
Contributor

yevdyko commented Oct 7, 2022

@jamesrweb could the last commit be a separate PR because it's related to the structure topic?

@yevdyko
Copy link
Contributor

yevdyko commented Oct 7, 2022

@jamesrweb What about having 3 files: index, 2 component files and all related contracts inside the particular component file? This way we improve the structure, but avoid unnecessary abstractions and one-line files, which don't add value. How's that for a compromise?

We can also extract a huge comment from the index file in the repo issue, since that is the right place for it. WDYT?

@yevdyko
Copy link
Contributor

yevdyko commented Oct 7, 2022

@jamesrweb I guess this dependencies update script doesn't seem to work, does it?

@jamesrweb
Copy link
Member Author

@jamesrweb could the last commit be a separate PR because it's related to the structure topic?

The whole PR here is structural, right?

@jamesrweb
Copy link
Member Author

@jamesrweb What about having 3 files: index, 2 component files and all related contracts inside the particular component file? This way we improve the structure, but avoid unnecessary abstractions and one-line files, which don't add value. How's that for a compromise?

We can also extract a huge comment from the index file in the repo issue, since that is the right place for it. WDYT?

The reason it is in the code was actually because I thought that if anyone pulls or does cmd+click to view the source code then it would be clearer as to why we use as to stop anyone wasting time trying to remove it when it's unfixable thanks to TS itself 😅🙈. IDK, we could abstract the comment to an issue and then just link that issue in the code though I suppose, WDYT?

@jamesrweb
Copy link
Member Author

@jamesrweb I guess this dependencies update script doesn't seem to work, does it?

Seems not to even be triggered, I think GH is either having an issue or we didn't configure something 😥🤨... will look into it on a different PR / issue pair.

I saw your message about using renovate, I'll take a look and consider it over the weekend.

@jamesrweb
Copy link
Member Author

@jamesrweb What about having 3 files: index, 2 component files and all related contracts inside the particular component file? This way we improve the structure, but avoid unnecessary abstractions and one-line files, which don't add value. How's that for a compromise?

We can also extract a huge comment from the index file in the repo issue, since that is the right place for it. WDYT?

I would say that we either move in this direction or we keep it as it is.

I'm leaning to make the change but I'm mindful of your opinion and will consider it deeply. Perhaps a good middle ground would be to get another reviewer from the community to mediate, WDYT?

I want to keep everyone happy but eventually a decision has to be made and documented.

I'm not bothered what the decision is even if I have a bias right now, as long as it's reasoned, acceptable to all sides and clearly laid out.

Another thing to consider is of course contributors and end users. I think this new way is a bit clearer and cleaner TBH and will make code splitting far simpler for the v4 implementation but again, that's all subjective, right?

Let me know your thoughts either way 💭!

@jamesrweb jamesrweb requested a review from yevdyko October 21, 2022 04:49
@yevdyko yevdyko mentioned this pull request Oct 23, 2022
@jamesrweb
Copy link
Member Author

I would like to merge this in preparation for the v4 release. If there is a technical issue or something really blocking this then I am open to hearing it but if it is just a taste subject, which it seems to be, then I think the community should decide and if they really want it rolled back we can always do a minor release accordingly. WDYT?

@jamesrweb jamesrweb force-pushed the update-project-structure branch from 00ef829 to 03e0aa7 Compare November 2, 2022 20:02
@jamesrweb jamesrweb requested a review from a team November 24, 2022 14:07
@jamesrweb
Copy link
Member Author

@yevdyko can you review this within the coming week please?

Please also note issue #207 which is now linked appropriately in the files ready for version 4 being released once this is merged.

@jamesrweb jamesrweb merged commit ec88d57 into master Dec 2, 2022
@jamesrweb jamesrweb deleted the update-project-structure branch December 2, 2022 09:17
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