Refine E2E module from upstream feedback: optional TinyMCE + client ergonomics - #9
Merged
Merged
Conversation
Only the generate-tinymce-combined CI task needs TinyMCE; fixtures-only consumers should not have to drag it in. Move it from require to require-dev (so this module's own tests still run) plus a suggest entry, and guard the task with class_exists() so it skips cleanly when the package is absent instead of fataling on the TinyMCE classes.
- loadAndNavigate defaults request to page.request (which also shares the page's auth cookies); the signature becomes (page, fixture, request?). This is a breaking change from (page, request, fixture). - Add editorReadyTimeout option for the editor-ready wait (was a fixed Playwright default). - reset() now checks the JSON success envelope, consistent with load()/loadAll(). - Document that the Playwright host needs the module's Composer deps installed (the client ships as a vendor deep-import, not on npm), and how to compose load() + page.goto() for non-pages navigation.
Now that the module declares silverstripe/htmleditor-tinymce as an optional (require-dev + suggest) dependency, the containerised test suite no longer receives it: the harness installs the module as a path-repo dependency, and Composer never installs a dependency's require-dev. recipe-cms does not pull it either (verified), so GenerateTinyMCECombinedTask would skip and its test would fail. Mirror the pin into .docker/app/composer.json's require-dev, matching how the other module dev tools (phpstan, phpunit, rector, ...) are already duplicated there, so the container can exercise the task.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses four observations from the downstream (grid) team consuming this module.
Changes
Make
silverstripe/htmleditor-tinymceoptional (feedback #1)Only
GenerateTinyMCECombinedTaskneeds TinyMCE; a fixtures-only consumer should not drag it in. Moved fromrequire→require-devplus asuggestentry, and guarded the task body withclass_exists()so it skips cleanly when the package is absent instead of fataling.Note on the feedback's premise: the observation assumed "recipe-cms pulls it anyway" — that is not true for recipe-cms 6.x (verified: nothing in a recipe-cms project depends on
htmleditor-tinymce). This is consistent with SS6 making the HTML editor pluggable. It does not change the decision to make the dep optional, but it does mean the container test harness must pull TinyMCE explicitly (see below).Playwright client ergonomics (feedback #3, #4)
loadAndNavigatedefaultsrequesttopage.request(which also shares the page's auth cookies). Breaking: signature changes from(page, request, fixture)to(page, fixture, request?).editorReadyTimeoutoption for the editor-ready wait (previously a fixed Playwright default).reset()now checks the JSONsuccessenvelope, consistent withload()/loadAll().Documentation (feedback #2, #3b)
load()+page.goto()for navigation outside the CMS pages screen, rather than parameterizingloadAndNavigate's hardcoded path.Container test harness
The Docker harness installs the module as a path-repo dependency, and Composer never installs a dependency's
require-dev. Combined with the finding above (recipe-cms doesn't pull TinyMCE), the optional-dependency change would leave the container without TinyMCE, soGenerateTinyMCECombinedTaskwould skip and its test would fail. Mirroredsilverstripe/htmleditor-tinymceinto.docker/app/composer.json'srequire-dev, matching how the other module dev tools are already duplicated there.Verification (in-container)
tsctypecheck: pass