-
Notifications
You must be signed in to change notification settings - Fork 799
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
Blocks: load each block from a separate file. #11286
Conversation
As we work to add several new modules, the existing module/blocks.php is updated quite often. It causes a few issues: - The file keeps getting longer and longer. - The longer the file, the longer it takes to skim through it. - Everyone working on that file ends up having to rebase frequently because we are merging changes to that file often. - Keeping the file in sync with WordPress.com can be more work if someone does not commit their matching WordPress.com diff immediately after merging a Jetpack PR. Splitting things into different files makes things easier to maintain for everyone.
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: March 5, 2019. |
jeherve, Your synced wpcom patch D23984-code has been updated. |
Should these generic blocks even live under Asking the dumb question in case you didn't consider it. I understand there may be reason to keep it where it is.
This always concerned me 🙂 |
I did not consider it, so thanks for asking. I don't have a good answer here. Why not I suppose? I don't really have a preference here, so happy to change that if necessary. Just say the word :) |
It's even weirder on WP.com where I think the I'm all for moving things elsewhere, but I'm not sure what would be the best location on WP.com -- we should probably figure that out. |
BTW thanks for this. Updating (I mean it will be quite annoying to do it again if this one lands before, but at least things will be easier in the long run 😅 ) |
See #11286 (comment) I also took the opportunity to get rid of the blocks file entirely. It is not needed anymore, we can now move the loading of the blocks to the actual Gutenberg class.
jeherve, Your synced wpcom patch D23984-code has been updated. |
I placed them in |
Thanks for working on this @jeherve! |
Since we've talked about the blocks' location, here is how I would personally envision the blocks's file structure in the future:
|
Not saying we should always follow core's patterns but just for the record and awereness:
|
@simison I like it! That actually makes a lot of sense, especially in the context of publishing every block to npm separately. On our end, that structure would probably be easier to understand for new contributors once we bring the blocks's JSX to Jetpack. I have 2 questions about this approach:
|
Great questions!
I would probably try to re-align with the rest of Jetpack and:
I recognize these are not something we can do now but it's good to envision a bit further into the future at this point.
I think it's okay; that's basically how core blocks work, too. They rely on APIs and functionality from other files (think search block, recent posts block, or upcoming comments block). I'd expect there to be at least one PHP file for each block where we'd actually define at least dependencies for the block so that at least those would be in the same place with JS sources. I would also take the chance and rename the root folder to something else than How does that sound? |
On 2nd thought, we might also want to eventually phase out "gutenberg" term (and use "block editor" instead) so worth considering if we could avoid introducing it. Core has been doing similar changes recently. So |
Just some backround info as to why I placed blocks into It was mostly done for consistency. There is also some precedent on code that isn't exactly a module to live in that directory. So my suggestion is to not have a special folded called |
I haven't tested, but this seems to me like a nice change that will benefit developers. I like to know where things should go and understand why 🙂 Should all this live in one file? It's pretty clear to me that it should not. The whopping 7000+ line Should we call it blocks? 🤷♂️ I'm not sure. At this point it all seems to be all blocks. Seems descriptive at this time, that may change, naming things is hard. Good enough for now. Should it live under modules? 🤷♂️ What's a module? Jetpack modules seem to be a specific thing with some expected behavior. The fact that it's strange enough that we need to mention in the header "Hey, this isn't actually a module" indicates to me that the location should raise some red flags. Maybe blocks are modules? Why not? That's the paradigm for additional functionality that Jetpack has. Why not just make each block a module? It's probably just overhead, but these are the kind of lines we can draw that helps me to work effectively. If the contract with Jetpack is that it provides a core that modules can extend, maybe that's exactly what we should be doing. This has gotten a bit philosophical so I'll bring it back home. I'm in favor of this change, but @Automattic/jetpack-crew should have the final say. At the end of the day, I don't care too much exactly where the files live, every decision has tradeoffs. I do think it's important we reason about why we make the decisions we do and consider alternatives. |
@dereksmart Could you weigh in on this maybe? Thank you! |
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.
I haven't had time to test this, but I wanted to manifest my support of the general direction taken here.
I'll reference my more rambling thoughts with greater detail in another comment (#11286 (comment))
Closing in favor of #11312, as the rebase was a nightmare :( |
Changes proposed in this Pull Request:
As we work to add several new modules, the existing module/blocks.php is updated quite often. It causes a few issues:
Splitting things into different files makes things easier to maintain for everyone.
Internal reference: pafL3P-io-p2
Testing instructions:
Proposed changelog entry for your changes: