-
Notifications
You must be signed in to change notification settings - Fork 49
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
Conversation
91a90d7
to
0605c2d
Compare
@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 👍🏻 |
There was a problem hiding this 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?
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 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 🚀 |
Why not have the main component in the index if the entire library consists of that component? |
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. |
@jamesrweb could the last commit be a separate PR because it's related to the structure topic? |
@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 |
@jamesrweb I guess this dependencies update script doesn't seem to work, does it? |
The whole PR here is structural, right? |
The reason it is in the code was actually because I thought that if anyone pulls or does |
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. |
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 💭! |
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? |
00ef829
to
03e0aa7
Compare
Proposed Changes
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 newindex.tsx
as those within the current index.tsx.