Skip to content
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

Bootstrap the architecture documentation #22751

Merged
merged 7 commits into from
Jun 8, 2020
Merged

Conversation

youknowriad
Copy link
Contributor

I feel like sometimes we repeat ourselves explaining some of the technical decisions on the project and this might come down to the lack of "architecture documentation" on the block editor repository.

This documentation can be very valuable for folks new to the repository or that want to learn more and contribute more heavily.

This PR is an attempt to start this documentation. For now I added two documents:

@youknowriad youknowriad added the [Type] Developer Documentation Documentation for developers label May 29, 2020
@youknowriad youknowriad self-assigned this May 29, 2020
@youknowriad youknowriad requested review from aduth, mtias and mkaz May 29, 2020 14:44

## Going further

- [Journey towards a performant editor](https://riad.blog/2020/02/14/a-journey-towards-a-performant-web-editor/)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it's ok to have a link to my blog like that but I felt that that post doesn't make sense as is in the docs but that it's still good to learn more about the perf work.

Copy link
Contributor

Choose a reason for hiding this comment

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

My two cents is that it's okay to have for now as a V1 :)

Copy link
Member

Choose a reason for hiding this comment

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

An external link policy was discussed in a recent docs meeting and the basic summary: ideally we enhance official docs, but if valuable external resource exists it is ok to link.

Copy link
Member

Choose a reason for hiding this comment

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

Copy and paste then? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine copy/pasting like I did for the "modularity" doc, this blog post link here just don't fit in the docs IMO

- [Block Editor Performance](docs/architecture/performance.md).
- What are the decision decisions behind the Data Module?
- Why puppeteer is the tool of choice for End 2 End tests?
- What's the difference between the different editor packages? and what's the purpose of each package?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hoping that by starting this, other folks can chime-in and document other decisions made on the repo.

@github-actions
Copy link

github-actions bot commented May 29, 2020

Size Change: +17.4 kB (1%)

Total Size: 1.13 MB

Filename Size Change
build/a11y/index.js 1.14 kB +2 B (0%)
build/annotations/index.js 3.62 kB +3 B (0%)
build/api-fetch/index.js 3.4 kB +2 B (0%)
build/autop/index.js 2.83 kB +1 B
build/block-directory/index.js 6.77 kB +292 B (4%)
build/block-directory/style-rtl.css 892 B +105 B (11%) ⚠️
build/block-directory/style.css 892 B +105 B (11%) ⚠️
build/block-editor/index.js 106 kB +692 B (0%)
build/block-editor/style-rtl.css 11.4 kB +63 B (0%)
build/block-editor/style.css 11.4 kB +61 B (0%)
build/block-library/editor-rtl.css 7.87 kB +260 B (3%)
build/block-library/editor.css 7.87 kB +262 B (3%)
build/block-library/index.js 127 kB +8.19 kB (6%) 🔍
build/block-library/style-rtl.css 7.72 kB +40 B (0%)
build/block-library/style.css 7.72 kB +36 B (0%)
build/block-serialization-default-parser/index.js 1.88 kB -1 B
build/blocks/index.js 48.1 kB +11 B (0%)
build/components/index.js 194 kB +4.38 kB (2%)
build/components/style-rtl.css 19.5 kB +9 B (0%)
build/components/style.css 19.5 kB +9 B (0%)
build/compose/index.js 9.31 kB -3 B (0%)
build/core-data/index.js 11.4 kB +13 B (0%)
build/data/index.js 8.45 kB +27 B (0%)
build/date/index.js 5.47 kB +6 B (0%)
build/dom-ready/index.js 569 B +1 B
build/dom/index.js 3.17 kB +55 B (1%)
build/edit-navigation/index.js 8.25 kB +366 B (4%)
build/edit-navigation/style-rtl.css 918 B +61 B (6%) 🔍
build/edit-navigation/style.css 919 B +63 B (6%) 🔍
build/edit-post/index.js 303 kB +243 B (0%)
build/edit-site/index.js 15.5 kB +1.41 kB (9%) 🔍
build/edit-widgets/index.js 9.34 kB +507 B (5%) 🔍
build/editor/index.js 44.7 kB +77 B (0%)
build/format-library/index.js 7.72 kB +12 B (0%)
build/hooks/index.js 2.13 kB -3 B (0%)
build/html-entities/index.js 621 B -1 B
build/i18n/index.js 3.56 kB +1 B
build/keyboard-shortcuts/index.js 2.51 kB +3 B (0%)
build/keycodes/index.js 1.94 kB +1 B
build/list-reusable-blocks/index.js 3.12 kB -2 B (0%)
build/media-utils/index.js 5.3 kB +10 B (0%)
build/notices/index.js 1.79 kB +2 B (0%)
build/nux/index.js 3.41 kB +3 B (0%)
build/plugins/index.js 2.56 kB -3 B (0%)
build/primitives/index.js 1.5 kB -1 B
build/redux-routine/index.js 2.85 kB -3 B (0%)
build/rich-text/index.js 14.8 kB +11 B (0%)
build/server-side-render/index.js 2.68 kB +4 B (0%)
build/token-list/index.js 1.28 kB -1 B
build/url/index.js 4.06 kB +39 B (0%)
build/viewport/index.js 1.85 kB +9 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/blob/index.js 620 B 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/deprecated/index.js 771 B 0 B
build/edit-post/style-rtl.css 5.43 kB 0 B
build/edit-post/style.css 5.43 kB 0 B
build/edit-site/style-rtl.css 2.96 kB 0 B
build/edit-site/style.css 2.96 kB 0 B
build/edit-widgets/style-rtl.css 2.4 kB 0 B
build/edit-widgets/style.css 2.4 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/style-rtl.css 4.26 kB 0 B
build/editor/style.css 4.27 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/priority-queue/index.js 789 B 0 B
build/shortcode/index.js 1.7 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@mkaz
Copy link
Member

mkaz commented May 30, 2020

Do you think this would fit in the "Project Overview" section of the docs?
https://developer.wordpress.org/block-editor/principles/

There are so related docs there, I can see a sub-section for Architecture fitting nicely.

See the table of contents as published at: https://developer.wordpress.org/block-editor/ the file structure in the docs doesn't necessarily match the published table of contents due to how docs have grown and been organized over time.

@youknowriad
Copy link
Contributor Author

@mkaz While there's definitely overlap when it comes to the decisions behind the block concept (and maybe better moved to the architecture docs), I do think the "architecture" docs are different and more technical and concrete like the examples on the index file while the "project overview" as being more "marketting/high-level".

@mkaz
Copy link
Member

mkaz commented Jun 1, 2020

@youknowriad I would rather see the Project Overview turn into the architecture documents, the "marketing and higher-level" docs were needed when the project was younger and needed to justify itself. I think the more technical docs are a better fit there now as the project matures.

@youknowriad
Copy link
Contributor Author

Makes sense @mkaz do you think you can help move this forward? I think you have a better context than me to make better-informed decisions about the potential reorganisation.

docs/architecture/index.md Outdated Show resolved Hide resolved
- [Modularity and WordPress Packages](docs/architecture/modularity.md).
- [Block Editor Performance](docs/architecture/performance.md).
- What are the decision decisions behind the Data Module?
- [Why is Puppeteer the tool of choice for end-to-end tests?](/docs/architecture/automated-testing.md)
Copy link
Member

Choose a reason for hiding this comment

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

In dd33523, I added a document addressing this question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks these are great.

- [Block Editor Performance](docs/architecture/performance.md).
- What are the decision decisions behind the Data Module?
- [Why is Puppeteer the tool of choice for end-to-end tests?](/docs/architecture/automated-testing.md)
- [What's the difference between the different editor packages? What's the purpose of each package?](/docs/architecture/modularity.md#whats-the-difference-between-the-different-editor-packages-whats-the-purpose-of-each-package)
Copy link
Member

Choose a reason for hiding this comment

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

In 25d906e, I added a section to the existing "Modularity" document to outline the purpose of the editor packages.


These include:

- **Interoperability with existing testing framework**. Puppeteer is "just" a tool for controlling a Chrome browser, and makes no assumptions about how it's integrated into a testing environment. While this requires some additional effort in ensuring the test environment is available, it also allows for cohesion in how it integrates with an existing setup. Gutenberg is able to consistently use Jest for both unit testing and end-to-end testing. This is contrasted with other solutions like Cypress, which provide their own testing framework and assertion library as part of an all-in-one solution.
Copy link
Contributor Author

@youknowriad youknowriad Jun 3, 2020

Choose a reason for hiding this comment

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

I think now it supports Firefox by default too.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it does 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I wasn't sure how cohesive I could mention that here, without it overcomplicating the messaging. And the support is still pretty early. Puppeteer still markets itself as "Headless Chrome Node.js API", and "a Node library which provides a high-level API to control Chrome or Chromium over the DevTools Protocol. Puppeteer runs headless by default, but can be configured to run full (non-headless) Chrome or Chromium." The text largely reflects that.

Copy link
Member

@aduth aduth Jun 3, 2020

Choose a reason for hiding this comment

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

I guess I kinda alluded to Firefox ("not just Chrome") later in the document, "largely":

The fact that Puppeteer largely targets the Chrome browser is non-ideal in how it does not provide full browser coverage.

- **An expressive but predictable API**. Puppeteer strikes a nice balance between low-level access to browser behavior, while retaining an expressive API for issuing and awaiting responses to those commands using modern JavaScript [`async` and `await` syntax](https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Asynchronous/Async_await). This is contrasted with other solutions, which either don't support or leverage native language async functionality, don't expose direct access to the browser, or leverage custom domain-specific language syntaxes for expressing browser commands and assertions. The fact that Puppeteer largely targets the Chrome browser is non-ideal in how it does not provide full browser coverage. On the other hand, the limited set of browser targets offers more consistent results and stronger guarantees about how code is evaluated in the browser environment.
- **Surfacing bugs, not obscuring them**. Many alternative solutions offer options to automatically await settled network requests or asynchronous appearance of elements on the page. While this can serve as a convenience in accounting for unpredictable delays, it can also unknowingly cause oversight of legitimate user-facing issues. For example, if an element will only appear on the page after some network request or computation has completed, it may be easy to overlook that these delays can cause unpredictable and frustrating behavior for users ([example](https://github.com/WordPress/gutenberg/pull/11287)). Given that developers often test on high-end hardware and stable network connections, consideration of resiliency on low-end hardware or spotty network availability is not always on the forefront of one's considerations. Puppeteer forces us to acknowledge these delays with explicit `waitFor*` expressions, putting us in much greater alignment with the real-world experience of an end-user.
- **Debugging**. It's important that in that case that a test fails, there should be straight-forward means to diagnose and resolve the issue. While its offerings are rather simplistic relative to the competition, Puppeteer does expose options to run tests as "headful" (with the browser visible) and with delayed actions. Combined with the fact that it interoperates well with native language / runtime features (e.g. debugger statements or breakpoints), this provides developers with sufficient debugging access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we could mention that Cypress didn't support keyboard interactions properly, not sure if it's still the case though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind just saw the link to the PR which describes these.

Copy link
Member

Choose a reason for hiding this comment

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

I'd initially taken an angle of describing the history of end-to-end testing in Gutenberg, and more specifically "Why we don't use Cypress" and describing all its faults, including what you describe as some of the browser emulation issues. I eventually reoriented it to a more positive "What requirements we want from our testing tool" direction.

But yes, the linked pull request can help illustrate some of those more specific examples.

Co-authored-by: Andrew Duthie <andrew@andrewduthie.com>
@youknowriad
Copy link
Contributor Author

I think there are already some very good docs here. @mkaz do you think we should move forward as is and restructure separately or should we do it all at once?

These include:

- **Interoperability with existing testing framework**. Puppeteer is "just" a tool for controlling a Chrome browser, and makes no assumptions about how it's integrated into a testing environment. While this requires some additional effort in ensuring the test environment is available, it also allows for cohesion in how it integrates with an existing setup. Gutenberg is able to consistently use Jest for both unit testing and end-to-end testing. This is contrasted with other solutions like Cypress, which provide their own testing framework and assertion library as part of an all-in-one solution.
- **An expressive but predictable API**. Puppeteer strikes a nice balance between low-level access to browser behavior, while retaining an expressive API for issuing and awaiting responses to those commands using modern JavaScript [`async` and `await` syntax](https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Asynchronous/Async_await). This is contrasted with other solutions, which either don't support or leverage native language async functionality, don't expose direct access to the browser, or leverage custom domain-specific language syntaxes for expressing browser commands and assertions. The fact that Puppeteer largely targets the Chrome browser is non-ideal in how it does not provide full browser coverage. On the other hand, the limited set of browser targets offers more consistent results and stronger guarantees about how code is evaluated in the browser environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also note that Cypress doesn't natively support the fetch API.

@youknowriad
Copy link
Contributor Author

Seems like we're ready to ship the initial version for this. Any approval?

@mtias
Copy link
Member

mtias commented Jun 4, 2020

This is a good initiative. I'd also like to see us using more inline comments contextual to the code or entry points like #20257

Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

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

👍 This is a great start, I committed a change that moved it up under Project Overview. I think this is a good section for it and it doesn't grow the sidebar, which is a problem we ran into before.

We can probably remove and clean up old stuff under Project Overview but can be addressed in other PRs.

@aduth
Copy link
Member

aduth commented Jun 5, 2020

I think I neglected to include the new automated testing document in the table of contents / manifest.

@youknowriad youknowriad merged commit bc51ec1 into master Jun 8, 2020
@youknowriad youknowriad deleted the doc/architecture branch June 8, 2020 11:24
@github-actions github-actions bot added this to the Gutenberg 8.3 milestone Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants