chore: use newspack-scripts #286

dkoo · 2022-01-19T22:55:14Z

Update CI images and use newspack-scripts as implemented in Automattic/newspack-plugin#1368 and https://github.com/Automattic/newspack-new-plugin-boilerplate/pull/3.

Also implements Dependabot, which was somehow missed during our last round adding it to all Newspack repos.

How to test the changes in this Pull Request:

Make sure build and lint commands execute successfully:
1. npm start (this is an alias for npm ci && npm run watch)
2. npm run build
3. npm run lint:js
4. npm run lint:scss
Verify commitizen works by staging a change and running npm run cm – a prompt for a structured commit message should appear, just line on master
Verify the commit-msg hook does lint the commit message:
1. reinstall the hook – composer install
2. stage a change and try to commit with an unstructured commit message (e.g. git commit -m "hehe") and observe it failing.

dkoo · 2022-01-19T22:58:26Z

@miguelpeixe I could use your help with a build error that I'm encountering with this branch. After installing newspack-scripts and moving to its configs, the build and watch commands fail to compile because of the extra babel config required by prebid.js. I get a bunch of errors from webpack like this:

ERROR in ./node_modules/prebid.js/modules/medianetRtdProvider.js
Module build failed (from ./node_modules/babel-loader/lib/index.js):
Error: /Users/dkoo/Local Sites/newspack/app/public/wp-content/plugins/newspack-ads/node_modules/prebid.js/modules/medianetRtdProvider.js: [BABEL] unknown: Preset /* your preset */ requires a filename to be set when babel is called directly,

babel.transform(code, { filename: 'file.ts', presets: [/* your preset */] });

See https://babeljs.io/docs/en/options#filename for more information.
    at validateIfOptionNeedsFilename (/Users/dkoo/Local Sites/newspack/app/public/wp-content/plugins/newspack-ads/node_modules/@babel/core/lib/config/full.js:295:11)
    at /Users/dkoo/Local Sites/newspack/app/public/wp-content/plugins/newspack-ads/node_modules/@babel/core/lib/config/full.js:307:52
    at Array.forEach (<anonymous>)
    at validatePreset (/Users/dkoo/Local Sites/newspack/app/public/wp-content/plugins/newspack-ads/node_modules/@babel/core/lib/config/full.js:307:25)
    at loadPresetDescriptor (/Users/dkoo/Local Sites/newspack/app/public/wp-content/plugins/newspack-ads/node_modules/@babel/core/lib/config/full.js:314:3)
    at loadPresetDescriptor.next (<anonymous>)
    at recursePresetDescriptors (/Users/dkoo/Local Sites/newspack/app/public/wp-content/plugins/newspack-ads/node_modules/@babel/core/lib/config/full.js:114:30)
    at recursePresetDescriptors.next (<anonymous>)
    at recursePresetDescriptors (/Users/dkoo/Local Sites/newspack/app/public/wp-content/plugins/newspack-ads/node_modules/@babel/core/lib/config/full.js:137:32)
    at recursePresetDescriptors.next (<anonymous>)
 @ ./src/prebid/index.js 7:0-47

ERROR in ./node_modules/prebid.js/src/adapters/bidderFactory.js 212:10-21
export 'default' (imported as 'events') was not found in '../events.js' (module has no exports)
 @ ./node_modules/prebid.js/src/adapterManager.js 10:0-56 553:23-32
 @ ./node_modules/prebid.js/src/prebid.js 24:21-62
 @ ./src/prebid/index.js 1:0-29 9:0-17

Removing the babel.config.js file added by this PR allows webpack to complete the build, but then all lint commands fail due to a missing babel config file like this:

Parsing error: No Babel config file detected for /Users/dkoo/Local Sites/newspack/app/public/wp-content/plugins/newspack-ads/src/blocks/ad-unit/editor.js. Either disable config file checking with requireConfigFile: false, or configure Babel so that it can find the config files

It's either one or the other. How do we resolve this conflict so we can both build and lint?

dkoo · 2022-01-24T18:39:18Z

@miguelpeixe would 53f1d85 fix it? Since we're bundling prebid files with their own separate entrypoint, if we only apply the prebid.js Babel presets and plugins to that entrypoint, would that keep things separate enough? This change lets me both build and lint successfully, and things seem to still run okay.

UPDATE: Confirmed that this does not fix the issue. From Miguel:

The bug we’re getting seems related to the '@babel/preset-typescript' preset from the calypso’s default.js webpack babel setup. Adding configFile: false to the prebid script rule options should be able to fix it, but it’s not. That’s what I’m struggling with 😞

To test, you can open the node_modules/@automattic/calypso-build/babel/default.js and comment-out the require.resolve( '@babel/preset-typescript' ), line to confirm it builds without it.

Reverted 53f1d85.

dkoo · 2022-01-24T21:09:09Z

Rebased this branch, so please re-pull from the remote. In order to avoid these errors, we're not using the instance of @automattic/calypso-build or eslint and its plugins from newspack-scripts.

miguelpeixe

Don't know exactly why, but commitlint is not being installed:

$ git commit -m "message"
✔ Preparing lint-staged...
✔ Running tasks for staged files...
✔ Applying modifications from tasks...
✔ Cleaning up temporary files...
.git/hooks/commit-msg: line 3: ./node_modules/.bin/commitlint: No such file or directory

dkoo · 2022-01-25T16:52:58Z

~~@miguelpeixe does e47387e fix it?~~

Scratch that, I think we were missing a commitlint config file. 6cd7025 adds that—any luck now?

miguelpeixe · 2022-01-25T19:22:46Z

Using composer install wasn't updating the commit hook, I had to run composer update which fixed the hook and also updated my composer.lock. Should we update it for this PR?

The commit command is working now but it's also allowing me to commit with an unstructured commit message. Is your hook working properly?

dkoo · 2022-01-25T23:47:20Z

The commit command is working now but it's also allowing me to commit with an unstructured commit message. Is your hook working properly?

Confirmed—the hook is not running. The following also outputs nothing:

echo 'hehe' | ./node_modules/.bin/newspack-scripts commitlint

Updating @wordpress/* dependencies fixes this, but also causes the lint error mentioned above:

Parsing error: No Babel config file detected for /Users/dkoo/Local Sites/newspack/app/public/wp-content/plugins/newspack-ads/src/blocks/ad-unit/editor.js. Either disable config file checking with requireConfigFile: false, or configure Babel so that it can find the config files

So we may as well use newspack-scripts for all dependencies (as with the other repos) and solve for the conflict with prebid.js's Babel plugin. 60932bf does this, and I can confirm the hook now runs as expected. But at the moment build and watch commands will fail due to the conflict. Tagging the PR as WIP until we figure this out.

dkoo · 2022-01-25T23:52:03Z

cc @adekbadek in case you have some insight here, too.

miguelpeixe · 2022-01-26T15:11:54Z

There's a solution to resolve the conflict between @babel/preset-typescript and pbjsGlobals plugin on Prebid.js' side.

I sent a PR with the fix. Meanwhile, I believe we should keep our own copy of this small babel plugin. WDYT @adekbadek @dkoo ?

miguelpeixe · 2022-01-26T15:34:06Z

2e42f5a adds the local pbjsGlobals.js ignoring lint to preserve the original file structure.
840f9b7 implements it to our webpack config, customizing the original .babelrc provided by Prebid.js.

dkoo · 2022-01-26T16:56:09Z

I can confirm that @miguelpeixe's fix with the local pbjsGlobals file allows us to build and lint, and the Prebid JS is running on the front-end. Looks 👍 to me!

See Automattic/newspack-scripts#50

adekbadek · 2022-01-27T10:55:54Z

Adding a fix for Automattic/newspack-scripts#50 in 1dcb33e

adekbadek · 2022-01-27T11:28:52Z

9b680d6 should resolve the commitlint issues

matticbot · 2022-01-27T17:09:23Z

🎉 This PR is included in version 1.26.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot · 2022-01-31T18:40:45Z

🎉 This PR is included in version 1.26.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

dkoo requested a review from a team January 19, 2022 22:55

dkoo self-assigned this Jan 19, 2022

dkoo added the On Hold label Jan 19, 2022

dkoo changed the title ~~Chore/use newspack scripts~~ chore: use newspack-scripts Jan 19, 2022

chore: use newspack-scripts, but not for build or eslint commands

a2f2369

dkoo force-pushed the chore/use-newspack-scripts branch from ccb6835 to a2f2369 Compare January 24, 2022 20:54

dkoo added 2 commits January 24, 2022 14:00

chore: remove commitlint config in favor of one in newspack-scripts

498d6f2

chore: restore stylelint dependencies

2e30c2b

dkoo marked this pull request as ready for review January 24, 2022 21:06

dkoo added [Status] Needs Review and removed On Hold labels Jan 24, 2022

miguelpeixe requested changes Jan 25, 2022

View reviewed changes

miguelpeixe added [Status] Needs feedback or changes and removed [Status] Needs Review labels Jan 25, 2022

fix: commitlint command in precommit hook

e47387e

chore: re-correct precommit hook script and add commitlint config file

6cd7025

dkoo requested a review from miguelpeixe January 25, 2022 17:13

dkoo added 6 commits January 25, 2022 14:44

chore: update composer dependencies

5028f26

chore: use newspack-scripts dependencies

60932bf

fix: composer dependencies

8b91f45

fix: composer dependencies

91fcba1

fix: composer dependencies

24c49a9

fix: eslint

0ac8868

dkoo changed the title ~~chore: use newspack-scripts~~ [WIP] chore: use newspack-scripts Jan 25, 2022

dkoo added Do not merge [Status] WIP and removed [Status] Needs feedback or changes labels Jan 25, 2022

chore: remove commented-out code

5bb1e3d

miguelpeixe self-assigned this Jan 26, 2022

miguelpeixe added 2 commits January 26, 2022 12:27

fix: use local pbjsGlobals.js

2e42f5a

fix: implement local pbjsGlobals.js

840f9b7

miguelpeixe requested a review from adekbadek January 26, 2022 15:36

miguelpeixe added [Status] Needs Review and removed [Status] WIP Do not merge labels Jan 26, 2022

miguelpeixe changed the title ~~[WIP] chore: use newspack-scripts~~ chore: use newspack-scripts Jan 26, 2022

ci: fix post-release flow

1dcb33e

See Automattic/newspack-scripts#50

chore: use newspack-scripts' commitlint

9b680d6

adekbadek approved these changes Jan 27, 2022

View reviewed changes

github-actions bot added [Status] Approved and removed [Status] Needs Review labels Jan 27, 2022

Merge branch 'master' into chore/use-newspack-scripts

706c9a2

dkoo merged commit 8961608 into master Jan 27, 2022

dkoo deleted the chore/use-newspack-scripts branch January 27, 2022 17:04

matticbot added the released on @alpha label Jan 27, 2022

matticbot added the released label Jan 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use newspack-scripts #286

chore: use newspack-scripts #286

dkoo commented Jan 19, 2022 •

edited

Loading

dkoo commented Jan 19, 2022

dkoo commented Jan 24, 2022 •

edited

Loading

dkoo commented Jan 24, 2022

miguelpeixe left a comment

dkoo commented Jan 25, 2022 •

edited

Loading

miguelpeixe commented Jan 25, 2022

dkoo commented Jan 25, 2022 •

edited

Loading

dkoo commented Jan 25, 2022

miguelpeixe commented Jan 26, 2022

miguelpeixe commented Jan 26, 2022

dkoo commented Jan 26, 2022

adekbadek commented Jan 27, 2022

adekbadek commented Jan 27, 2022

matticbot commented Jan 27, 2022

matticbot commented Jan 31, 2022

chore: use newspack-scripts #286

chore: use newspack-scripts #286

Conversation

dkoo commented Jan 19, 2022 • edited Loading

How to test the changes in this Pull Request:

dkoo commented Jan 19, 2022

dkoo commented Jan 24, 2022 • edited Loading

dkoo commented Jan 24, 2022

miguelpeixe left a comment

Choose a reason for hiding this comment

dkoo commented Jan 25, 2022 • edited Loading

miguelpeixe commented Jan 25, 2022

dkoo commented Jan 25, 2022 • edited Loading

dkoo commented Jan 25, 2022

miguelpeixe commented Jan 26, 2022

miguelpeixe commented Jan 26, 2022

dkoo commented Jan 26, 2022

adekbadek commented Jan 27, 2022

adekbadek commented Jan 27, 2022

matticbot commented Jan 27, 2022

matticbot commented Jan 31, 2022

dkoo commented Jan 19, 2022 •

edited

Loading

dkoo commented Jan 24, 2022 •

edited

Loading

dkoo commented Jan 25, 2022 •

edited

Loading

dkoo commented Jan 25, 2022 •

edited

Loading