Introduce register_asset() and enqueue_asset() functions; refactor & test plugin #22

kadamwhite · 2020-07-09T02:57:57Z

This PR proposes a new API for inclusion in v0.4, oriented around a more WordPress-style (see #21) single-asset function signature of

Asset_Loader\enqueue_asset(
    $manifest_path,
    'target-asset.js',
    [
        'dependencies' => [ 'wp-data' ],
        'handle' => 'awesome-theme-styles',
    ]
);

This PR simultaneously (sorry!) introduces some refactoring and renaming of existing internal methods, and adds a number of unit tests across the project.

Needs testing and additional tests prior to 0.4, but this is now available for review & critique.

Tagging @svandragt @johnbillion and @mikeselander as thanks for your constructive criticism on other issues; let me know what you think!

kadamwhite · 2020-07-09T03:03:05Z

inc/namespace.php

+	// If we fail to match a .css asset, try again with .js in case there is a
+	// JS wrapper for that asset available (e.g. when using DevServer).
+	if ( empty( $asset_uri ) && is_css( $target_asset ) ) {
+		$asset_uri = Manifest\get_manifest_resource( $manifest_path, preg_replace( '/\.css$/', '.js', $target_asset ) );


This is the piece of this that I'm least confidant about: it solves #1 because you can say
register_asset( $manifest_path, 'some-file.css', [ 'handle' => 'my-css' ] );
but it's potentially unintuitive that this could lead to the enqueuing of a JS file in some circumstances.

There's also a small but non-zero chance that, in the event that you try to enqueue a CSS file while running the dev server, and you wish to enqueue the CSS in a different context or condition than the JS file from which the CSS will be extracted, that you could end up having the JS run in an unexpected situation if you try to enqueue the CSS without the JS (impossible when they haven't been split apart using MiniCSSExtractPlugin). I cannot think off hand of a time when this would practically happen on a project, but as much as moving away from autoenqueue distances us from the ✨ magic ✨ of this library, the inference of a JS file when you asked for CSS still is "magic" and may be confusing.

Or, am I over-thinking this 😁

This has tricky implications for dependency handling. We may need to revert to the scripts and styles arrays for dependencies rather than tracking it as one bundle, or else reinstate some of the $has_css logic from the prior versions of this function.

.editorconfig

inc/admin.php

inc/namespace.php

The logic of this function is not directly related to the logic we want to test, so mock it globally.

Webpack contenthash strings can be 20 characters when using Webpack defaults, which would not be long enough to trigger our full hashing heuristic. Since the average word length in most widespread languages across the globe is less than 15, and long words are rarely restricted to only a-f, our adapted heuristic should still be robust enough to catch hashed filenames with very limited false positives.

kadamwhite · 2020-07-21T04:19:48Z

@mikeselander (and @tfrommen, because I'm sure there's a better way to handle the testing for registered scripts!), would love your eyes on this if you would be so kind!

The last missing piece of functionality is that enqueueing a CSS file in a dev server environment does not work the same as registering such an asset. I want to dig on on that but I may like to get this landed and fix that bug separately, because this has gotten somewhat ambitious.

README.md

.travis.yml

tfrommen · 2020-07-21T05:33:33Z

composer.json

    },
    "scripts": {
-        "lint": "phpcs --standard=phpcs.ruleset.xml ."
+        "lint": "phpcs --standard=phpcs.ruleset.xml .",


Why is PHP_CodeSniffer expected as global dependency? Why is PHPUnit a local one (which I absolutely prefer)?

Since the config file has a default name, it will automagically get picked up. No need to specify:

Suggested change

"lint": "phpcs --standard=phpcs.ruleset.xml .",

"lint": "phpcs .",

This is not the same for me. If I do not manually specify the ruleset, it uses a completely different set of standards. Am I possibly using an invalid ruleset name?

Ah, right. Sorry, my bad. The supported name would be phpcs.xml(.dist). (Or others.)

Ahah! Thanks!

tfrommen · 2020-07-21T05:37:53Z

phpcs.ruleset.xml

-    <rule ref="vendor/humanmade/coding-standards">
-        <exclude name="Squiz.Commenting.FunctionComment.ParamCommentFullStop" />
-    </rule>
+	<rule ref="vendor/humanmade/coding-standards">


Reference the standard by name, not path:

Suggested change

<rule ref="vendor/humanmade/coding-standards">

<rule ref="HM">

tests/bootstrap.php

inc/manifest.php

tfrommen · 2020-07-21T12:24:22Z

tests/class-mock-asset-registry.php

+	public $enqueued;
+
+	public function __construct() {
+		$this->reset();


Why would you need to reset something on construction? Do you mis-use __construct somewhere in your code?

If not, I'd provide default values for the two properties/fields (and make them (private), and delete the constructor.

tfrommen · 2020-07-21T12:32:32Z

tests/inc/class-test-admin-scripts.php

+	 */
+	public function test_maybe_setup_ssl_cert_error_handling( bool $is_admin, string $script_uri, bool $expect_action, string $message ) : void {
+		WP_Mock::userFunction( 'is_admin' )->andReturn( $is_admin );
+		if ( $expect_action ) {


This is one of the examples where Brain Monkey provides some more useful helpers. It stubs the has_action function (using a fake registry), so your code here could be as simple as this:

$this->assertSame( $should_have_added_action, has_action( ... ) );

No need for a conditional and/or two different functions to check what you're interested in.

That sounds really appealing; swapping mock library's out of scope right now, but i'll look into it for later on@

tests/inc/class-test-asset-loader.php

Co-authored-by: Thorsten Frommen <[email protected]>

tests/bootstrap.php

Co-authored-by: Thorsten Frommen <[email protected]>

kadamwhite added 12 commits July 8, 2020 16:07

Use a static $manifests cache to avoid repeat file reads in one cycle

152349f

Use the new PHPCS ruleset in CI

2cc9705

Install PHPUnit

9d3ba7b

Begin moving admin script warning to new namespace, with tests

5d4d240

Expand test coverage of admin script load error notice handling

314c05f

Include tests in travis, and add editorconfig

b57c4b4

Add Manifest\get_manifest_resource method

d77b114

Add unit tests for Paths\plugin_or_theme_file_uri

fa62dc4

Rename path_or_theme_file_uri to get_file_uri

fe13729

Test is_css and adjust admin namespace test function comments

5e766a6

Introduce single-resource register_asset and enqueue_asset functions

32324f4

Disable side-effects rule in tests bootstrap

3e6a7a9

kadamwhite commented Jul 9, 2020

View reviewed changes

kadamwhite added 7 commits July 9, 2020 09:59

Complete moving ssl cert error logic to Admin namespace

b66dfa1

Name each test case in data providers

2220e57

Add _doing_it_wrong to autoenqueue/autoregister

51715ce

Consistently apply SSL cert error notification in all main logic paths

f45f962

Add CHANGELOG.md covering through upcoming 0.4

5c0dcbb

Use resource URI as script or style handle if no handle provided

3b5c0c6

Update README

213b4dd

mikeselander reviewed Jul 9, 2020

View reviewed changes

.editorconfig Show resolved Hide resolved

inc/admin.php Show resolved Hide resolved

inc/namespace.php Show resolved Hide resolved

inc/namespace.php Show resolved Hide resolved

kadamwhite mentioned this pull request Jul 9, 2020

Pass human-facing strings through i18n functions #23

Open

Introduce Manifest\get_version() helper to set asset version strings

2c2fab8

kadamwhite force-pushed the new-api branch from cc5dc8d to 2c2fab8 Compare July 20, 2020 22:20

kadamwhite added 6 commits July 20, 2020 18:30

Use new Manifest\get_version() function when registering assets

1cf2d0b

Move mocks of trailingslashit to base test class

e2327a4

The logic of this function is not directly related to the logic we want to test, so mock it globally.

Add overly-complex unit test setup for register_asset method

729305c

Add test for complex fallback logic for CSS-only dev assets

e5716bf

Eliminate trailing comma causing error in PHP 7.2

8b9e72c

kadamwhite requested review from tfrommen and mikeselander July 21, 2020 04:19

tfrommen reviewed Jul 21, 2020

View reviewed changes

kadamwhite and others added 7 commits July 21, 2020 11:38

Update inc/manifest.php

28ace0a

Co-authored-by: Thorsten Frommen <[email protected]>

Use terse forms of composer script commands

d881e63

Remove superfluous is_admin check

cd9611c

Rename phpcs file to remove need to manually specify --standard

6f545d6

Use null coalescing to obviate isset check

de59196

Co-authored-by: Thorsten Frommen <[email protected]>

Use is_readable over file_exists

93e3782

Co-authored-by: Thorsten Frommen <[email protected]>

Remove unnecessary namespace & "use"

6d070f5

Co-authored-by: Thorsten Frommen <[email protected]>

tfrommen reviewed Jul 21, 2020

View reviewed changes

tests/bootstrap.php Show resolved Hide resolved

tfrommen and others added 7 commits July 21, 2020 18:06

Remove superfluous blank line.

01936ab

Unset DOM loaded event after execution.

e9f6453

Co-authored-by: Thorsten Frommen <[email protected]>

Use rtrim in trailingslashit mock implementation

faf56db

Co-authored-by: Thorsten Frommen <[email protected]>

Remove admin flag from tests for function which no longer cares

31fa16b

Use strict_types=1 throughout codebase

6cf6f1b

Update CHANGELOG to reflect versioning logic changes

0609d7e

Clarify usage of the new methods in CHANGELOG

fd70cf2

kadamwhite merged commit 11c43e8 into main Jul 21, 2020

This was referenced Jul 21, 2020

Reference HM sniff by name #24

Merged

Document deprecations and clean up testing/CI code #25

Merged

Correct css enqueuing logic #26

Merged

kadamwhite deleted the new-api branch October 28, 2020 14:27

kadamwhite mentioned this pull request Oct 28, 2020

Support CSS-only entrypoints. #1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce register_asset() and enqueue_asset() functions; refactor & test plugin #22

Introduce register_asset() and enqueue_asset() functions; refactor & test plugin #22

kadamwhite commented Jul 9, 2020 •

edited

Loading

kadamwhite Jul 9, 2020

kadamwhite Jul 9, 2020

kadamwhite commented Jul 21, 2020

tfrommen Jul 21, 2020

kadamwhite Jul 21, 2020

tfrommen Jul 21, 2020 •

edited

Loading

kadamwhite Jul 21, 2020

tfrommen Jul 21, 2020

tfrommen Jul 21, 2020

tfrommen Jul 21, 2020

kadamwhite Jul 21, 2020

	"lint": "phpcs --standard=phpcs.ruleset.xml .",
	"lint": "phpcs .",

	<rule ref="vendor/humanmade/coding-standards">
	<rule ref="HM">

Introduce register_asset() and enqueue_asset() functions; refactor & test plugin #22

Introduce register_asset() and enqueue_asset() functions; refactor & test plugin #22

Conversation

kadamwhite commented Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadamwhite commented Jul 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfrommen Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadamwhite commented Jul 9, 2020 •

edited

Loading

tfrommen Jul 21, 2020 •

edited

Loading