Problem/Motivation

CKEditor 5 has its own build process. This should be integrated with the core build process in preparation to being moved to core.

See #3 for more nuance.

Proposed resolution

Combine package.json, js/admin/package.json, and js/drupal/package.json into a single package.json file which can be moved to the core/package.json in the MR that adds CKEditor 5 to core.

Issue fork ckeditor5-3227857

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Needs issue summary update

If I'm not mistaken, this means expanding this in core/package.json:

  "scripts": {
    "build": "yarn build:css & yarn build:js & yarn build:jqueryui",
    "watch": "yarn watch:css & yarn watch:js",
    "build:css": "cross-env BABEL_ENV=legacy node ./scripts/css/postcss-build.js",
    "watch:css": "cross-env BABEL_ENV=legacy node ./scripts/css/postcss-watch.js",
    "build:js": "cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js",
    "build:js-dev": "cross-env NODE_ENV=development BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js",
    "build:jqueryui": "cross-env NODE_ENV=development BABEL_ENV=legacy node ./scripts/js/jqueryui-build.js",

So probably that means adding build:ckeditor5?

Does this have other implications too?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

That will be one of the changes required. However, we might have to look deeper than that because at the moment, we have three package.json files in the module: package.json, js/admin/package.json, and js/drupal/package.json. Drupal core has a single package.json file for all of core. Ideally we would be able to move contents of all three files to that package.json.

lauriii opened merge request !95

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2591827: Add YAML linting to core coding standards checks

That fixed it! :D

But now there's a different failure:

Checking drupalci.yml

PHPCS: drupalci.yml passed

/var/www/html/modules/contrib/ckeditor5/drupalci.yml
  5:5  error  Empty mapping values are forbidden  yml/no-empty-mapping-value

✖ 1 problem (1 error, 0 warnings)

How … is that even possible?! Line 5 … did not change. Must be a new upstream requirement… yep: #2591827: Add YAML linting to core coding standards checks

Fixed in d22d96beddade3200a9aed82504393593542903d.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Unassigned » lauriii
Status: Active » Needs review

Green!

What's next here, @lauriii?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: lauriii » bnjmnm

Apparently nothing, so ready for final review from @bnjmnm!

bnjmnm made their first commit to this issue’s fork.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
Issue tags: -Needs issue summary update

  • bnjmnm committed b9e03ea on 1.0.x authored by lauriii 
    Issue #3227857 by lauriii, Wim Leers, bnjmnm: Integrate build process...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs review » Fixed

Feedback addressed, good consolidations!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.