-
Notifications
You must be signed in to change notification settings - Fork 63
Update versions in package.json, use @wordpress/scripts for build process #492
Conversation
Use the version on npmjs.com as a reference, for example: https://www.npmjs.com/package/npm-run-all
In webpack.config.js, migrate from extract-text-webpack-plugin to mini-css-extract-plugin, as it was deprecated and no longer available.
@see https://make.wordpress.org/core/2019/03/25/building-javascript/ This also required modifying webpack.config.js. It looks like the style.scss or style.css files are not used anywhere, so remove the rules for them.
I'm not sure how I missed these before. But this is simply from running the npm script.
@@ -57,19 +25,42 @@ module.exports = { | |||
loader: 'babel-loader', | |||
}, | |||
}, | |||
{ | |||
test: /style\.s?css$/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this plugin doesn't use style.css
or style.scss
. I couldn't find a reference to it, though maybe I missed something.
@@ -88,6 +88,7 @@ package-lock.json | |||
*.swp | |||
*.tmp | |||
*.bak | |||
*.map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new build process creates .map
files, like block-lab/css/blocks.editor.css.map
.
To test this, I've been deleting |
Also, restore the ^ to some npm packages. This will enable upgrades to minor releases, like from 0.15.0 to 0.16.0.
I still need to do a lot of regression testing here, as it upgrades almost every package in |
To be more in line with typical Gutenberg-based plugins.
I'm not sure if this is needed anymore, as we're mainly using the wordpress/scripts build system.
Thanks to the AMP plugin for this snippet.
Something like foo( 'baz', 'bar', ) looks strange. I'm not sure we need to have that. Also, this eslint configuration is now also used for a non-compiled file: admin.block-post.js And it looks like trailing commas in functions are not supported enough in browsers.
"error", | ||
{ | ||
"arrays": "always-multiline", | ||
"objects": "always-multiline", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires the trailing comma after 'bar' here:
const foo = {
one: 'baz',
two: 'bar',
};
...but doesn't allow a trailing comma after 'bar' here:
const foo = { one: 'baz', two: 'bar' };
…ents Instead of simply allowing this, require it.
"objects": "always-multiline", | ||
"imports": "always-multiline", | ||
"exports": "always-multiline", | ||
"functions": "never" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A trailing comma after the last argument looks strange to me: doThis( 'arg1', 'arg2', )
...and it doesn't have great browser support. This .eslintrc
is also used for a non-compiled file: admin.block-post.js
.
Ideally, soon that file will be ported to the React logic in the refactoring.
This isn't used anymore, so there's no need to require it.
{ | ||
"ignoreRestSiblings": true | ||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file could probably use more rules, but that's not in scope for this PR.
Apply fixes for the errors that this script produces.
Ensures that the dependencies have the same license as this plugin
CSS and JS Compilation Because this edits Here's a diff of the compiled Much of the diff is from vendor prefixes: So far, I didn't see a CSS or JS problem on: MacOS with Chrome 79 I tested all fields, including the repeater. It's possible that I missed a styling issue, but I didn't see anything obvious. |
Request For Review Hi @lukecarbis, It updates the packages in Regression testing of all of the fields looks good so far. Thanks, Luke! |
"block-editor", | ||
"wordpress", | ||
"wordpress-plugin" | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These edits are from the lint:pkg-json
script below.
@@ -2,50 +2,65 @@ | |||
"name": "block-lab", | |||
"title": "Block Lab", | |||
"version": "1.5.2", | |||
"description": "WordPress plugin with a simple templating system for building custom blocks.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to suggest something different, I added this because there was an error from not having a description.
Thanks to the AMP plugin for the syntax for the plugin. There was an issue reported in the Slack #support channel, where wp.nux.DotTip was moved to wp.components.Guide. By importing WP dependencies as modules, we can control the versions we're using. Also, use a dependency injection plugin. This will keep the dependencies up to date in wp_enqueue_script(). I don't like how it adds a PHP file to js/ But without that, there was a bug, with the console error: Uncaught TypeError: this.activateMode is not a function
There looks to be a second one, though I'm not sure why.
These mentioned that InspectorControls and InspectorAdvancedControls moved to block-editor.
We might consider how to replace this in a separate issue. But for now, I think it's appropriate to remove it.
@@ -1,8 +1,8 @@ | |||
/** | |||
* WordPress dependencies | |||
*/ | |||
const { applyFilters } = wp.hooks; | |||
const { select } = wp.data; | |||
import { applyFilters } from '@wordpress/hooks'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for importing these as modules instead of using something like wp.hooks
is so that we can control the module versions.
For example, InspectorControls
and InspectorAdvancedControls
were moved from wp.editor
to wp.block-editor
. And wp.nux
was deprecated entirely.
If we control the versions of the modules, we should be less dependent on the version of Core and Gutenberg users have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A user reported an error from wp.nux.DotTips
being deprecated in Gutenberg. I probably would have seen that if we imported that deprecated module.
Request For Review Hi @lukecarbis, 440f50a in this PR fixes an issue raised in the #support Slack channel, and in https://wordpress.org/support/topic/blocks-not-visible-in-admin-browser-console-error-wp-nux-not-defined/ Regression testing of all fields looks good. Though of course this is a huge diff. Thanks, Luke! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I don't understand most of this, but it mostly seems to work. The parts that don't could be my machine. After running npm install
, I also tried:
- npm run check-licenses ✅
- npm run dev ✅
- npm run build ✅
- npm run lint ❌ (
ERROR: Referenced sniff "WordPress-Core" does not exist
) - npm run lint:js ✅
- npm run lint:pkg-json ✅
- npm run lint:php ❌ (
ERROR: Referenced sniff "WordPress-Core" does not exist
)
FWIW, if I run
WordPress-Core is obviously installed on my system. |
Thanks for bringing that up, it's important that |
Hi @lukecarbis, rm -rf vendor && composer install && npm run lint:php There's a package in |
Just tried the following:
That fixed the problem. Note, I had to also remove the composer.lock file. Ready to merge! |
Ah, thanks. Good idea to remove |
Here's a build of the |
package.json
to the latest version in https://www.npmjs.com/. For example, https://www.npmjs.com/package/gulp-run.@wordpress/scripts
package in the build process. This will make Block Lab more in line with Gutenberg-based plugins.package-lock.json
file to the repo. This should ensure consistency of versions. For example, inpackage.json
, thegulp
version is^4.0.2
. The^
means that doingnpm install
can updategulp
to4.1.0
or4.2.0
if and when those exist. Sopackage-lock.json
should ensure that we have the same package tree.Closes #266