Repository navigation
Conversation
myabc
added
maintenance
skip changeset
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| export function renderTranslationsIndex(locales) { | ||
| const entries = [...locales] | ||
| .sort() | ||
| .map((locale) => ` [${JSON.stringify(locale)}, () => import(${JSON.stringify(`./${locale}.js`)})],`) |
There was a problem hiding this comment.
🟡 Changes recommended
The changeset gate, initial publication flow, and canary version uniqueness have unresolved release risks.
4 open findings
What changed in this PR
Makes the CKEditor build publishable as an npm package with automated release and canary workflows.
Changes:
- Builds and publishes bundle, styles, types, and lazy translations from
dist/. - Adds Changesets-based releases, canary publishing, and package verification.
- Updates local OpenProject integration, CI, documentation, and tests.
| File | Description |
|---|---|
README.md |
Documents build, development, and release workflows. |
package.json |
Defines package exports, scripts, and release tooling. |
package-lock.json |
Locks new publishing dependencies. |
jest.config.cjs |
Configures Jest for the ESM package. |
eslint.config.mjs |
Ignores build output and recognizes CJS configs. |
docker-compose.yml |
Mirrors builds into core’s installed package. |
CHANGELOG.md |
Records the 12.0.0 package changes. |
bin/verify-package.mjs |
Validates published tarball contents. |
bin/verify-build.mjs |
Verifies types and translation entry points. |
bin/mirror-to-core.mjs |
Copies builds into OpenProject core. |
bin/lib/translations-index.test.mjs |
Tests translation loading behavior. |
bin/lib/translations-index.mjs |
Generates lazy translation imports. |
bin/lib/release-workflow.test.mjs |
Checks Changesets action compatibility. |
bin/lib/paths.test.mjs |
Tests package mirroring paths. |
bin/lib/paths.mjs |
Centralizes build and core paths. |
bin/build.mjs |
Builds the ESM bundle into dist/. |
bin/build-ckeditor-translations.mjs |
Generates translation modules and index. |
bin/build-ckeditor-styles.mjs |
Writes package CSS into dist/. |
babel.config.cjs |
Configures Babel for TypeScript tests. |
.gitignore |
Excludes generated distribution files. |
.github/workflows/release.yml |
Adds release and canary publishing. |
.github/workflows/ci.yml |
Expands CI package verification. |
.github/workflows/changeset-check.yml |
Enforces changesets on pull requests. |
.changeset/README.md |
Documents changeset usage. |
.changeset/config.json |
Configures package versioning and changelogs. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| permissions: | ||
| contents: read |
| { | ||
| "name": "@openproject/commonmark-ckeditor-build", | ||
| "version": "v11.1.1", | ||
| "version": "12.0.0", |
| - name: Check for changeset | ||
| uses: brettcannon/check-for-changed-files@871d7b8b5917a4f6f06662e2262e8ffc51dff6d1 # v1 | ||
| with: | ||
| file-pattern: '.changeset/*.md' |
| printf -- '---\n"@openproject/commonmark-ckeditor-build": patch\n---\n\nCanary release\n' > .changeset/force-canary-release.md | ||
| jq '.changelog = "@changesets/cli/changelog"' .changeset/config.json > .changeset/config.tmp.json | ||
| mv .changeset/config.tmp.json .changeset/config.json | ||
| npx changeset version --snapshot canary |
|
Canary published: In npm install --save-exact @openproject/commonmark-ckeditor-build@0.0.0-canary-20261012095012 |
44ecd4b to
dfc5f86
Compare
f54c4bd to
eefec36
Compare
eefec36 to
d190cd8
Compare
oliverguenther
left a comment
oliverguenther
left a comment
There was a problem hiding this comment.
As mentioned in person and in element, I'm not yet convinced that we need to publish this to NPM. Yes, it improves consistency, but at the cost of developer efficiency. I'd welcome someone else to try out the DX after this change and see if you can still:
- Develop locally easily (currently watching/overriding the core build)
- Build and merge together with a core PR - This will likely be impossible as you first need a build, then a core PR
Especially the second point I'm not really convinced with. With our other dependencies, this makes sense as they are tested individually. For this build, we always depend on the core test suite for correctness.
/cc @opf/developers as other people worked on this more frequently than me in the past weeks.
A build no longer needs a core checkout. With OPENPROJECT_CORE set, the build is copied over the package core installed from npm, which keeps the local development loop without symlinks. https://community.openproject.org/wp/OP-20422
Core loads locales with a templated import of a relative path, which a bundler cannot expand for a package. The build now ships a static table of per-locale imports behind one function instead. The function also tries the lower-cased name and returns the name it loaded: core says zh-CN where CKEditor registers zh-cn, which left those users with an English editor. https://community.openproject.org/wp/OP-20422
Declares the entry points core needs and publishes only dist. The patch for the mention plugin moves from postinstall to prepare, since postinstall would also run, and fail, in a consumer's install. https://community.openproject.org/wp/OP-20422
Merging to master opens or updates a release pull request; merging that publishes to npm over OIDC. A pull request labelled canary gets a snapshot release, so core can test a change before it is released. https://community.openproject.org/wp/OP-20422
Describes building into dist, developing against a core checkout, and releasing with changesets and canary versions. https://community.openproject.org/wp/OP-20422
d190cd8 to
16ac9f3
Compare
|
I do agree with @oliverguenther. While I'm not against deploying to npm, I wouldn't be a fan of having a deployment step in between while developing locally. Maybe we can have both somehow? I wouldn't even mind adjusting the package.json in the OP frontend to something like or use npm link. |
|
I experience mixed feelings here. After going through my first PR relying on a change in CKEditor, I must say that it troubles me to commit a full
Like you, I’d prefer to have the ability to:
Sorry for the long comment, but I tried out a few options and… I reached both with this PR (replayed onto Develop locally. Build and merge together with a core PR. During review, the core PR pins the editor branch: Once the editor PR is merged, the core PR moves its pin to one of, depending on the editor’s release process:
The release tarball is the one I'd like to try if publishing to NPM is a problem. It's as light as a tag to release: a merge and a tag, no registry, npm account or trusted publisher. And it installs like an npm release: prebuilt, with no clone, devDependencies or build at install time. The lockfile records the tarball's integrity hash, so a replaced file makes installs fail instead of slipping in. That’s what I understand at least. What a core reviewer gets. Today, a vendored build shows up as
tl;dr: I find value here for core reviewers with explicit dependencies, and a good DX without regression — from where I sit. ℹ️ I use the Docker dev environment. Footnotes
|
Ticket
https://community.openproject.org/wp/OP-20422
What are you trying to accomplish?
Second of two PRs. It makes this repository publishable as
@openproject/commonmark-ckeditor-buildand adds the release flow, so that OpenProject core can replace the build committed underfrontend/src/vendor/ckeditor/with a pinned npm dependency. Stacked on the esbuild PR.After this, an editor change no longer needs build output copied into a core pull request. Core tests an unreleased change by pinning a canary version.
What approach did you choose and why?
Package
dist/and needs no core checkout. The package publishes onlydist/..),./ckeditor.css, and./translations.loadTranslation(locale)replaces core's templated import of a locale file. A bundler can only expand such an import for relative paths, not for a package, so the build ships a static table of per-locale imports that a consumer's bundler splits into lazy chunks.loadTranslationalso tries the lower-cased locale and resolves with the name it loaded. Core sayszh-CNandpt-BRwhere CKEditor registerszh-cnandpt-br; today those users get an English editor.windowglobals. The package deliberately declares no types for them: core declares them itself, and a second declaration of a different type would not compile. Real types across the boundary are a separate piece of work.postinstalltoprepare. Aspostinstallit would also run, and fail, inside a consumer's install.bin/verify-package.mjsasserts what would be published, and that no install script or runtime dependency reaches the consumer.publintchecks theexportsmap.12.0.0.licenseandcontributorsnow followLICENSE.md; the old values, and thebugsandrepositoryfields, came from the CKEditor build template.Release flow
opf/stimulus-elements: merging tomasteropens or updates a "Release Tracking" PR, and merging that publishes to npm over OIDC (Trusted Publishing, no token in the repository).canaryon a PR publishes a snapshot0.0.0-canary-<timestamp>under the dist-tagcanaryand comments with the install command. Only PRs from branches of this repository can do this.skip changesetbypasses it.Local development
npm run watchrebuilds intodist/. WithOPENPROJECT_COREset, each build is also copied over the package core installed, so no symlink is needed and core's nextnpm cirestores the pinned version.Open points
release.ymlby name, so anyone with write access can publish by changing that file in a PR.opf/primer_view_componentshas the same property.GITHUB_TOKENand therefore do not trigger CI.build (20.x)tobuild, since the Node matrix is gone. A branch protection rule naming the old check needs updating.package.jsonand the mention patch; whichever merges second needs a rebase.Verification
CI does not run for this PR while it targets the esbuild branch. Run locally:
npm run build,npm run verify:build,npm run verify:package,npm test(256 passed),npm run test:bin(13 passed),npm run lint,npm run typecheck. A cleannpm cistill applies the mention patch.AI involvement
Directed: written by Claude Code from a design agreed with @myabc, then reviewed adversarially by Codex. Codex found that the changesets action did not match the installed CLI version, which is fixed and covered by a test.