Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include check for JS build in addition to composer installation #1774

Merged
merged 2 commits into from Dec 27, 2018

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Member

westonruter commented Dec 21, 2018

For users who install from source, at the moment the plugin is only making sure that the user has run composer install. However, it also needs to check if the user has done npm install && npm run build. This makes that change.

@westonruter westonruter added this to the v 1.0.2 milestone Dec 21, 2018

@westonruter westonruter requested a review from felixarntz Dec 21, 2018

@googlebot googlebot added the cla: yes label Dec 21, 2018

Show resolved Hide resolved amp.php
@felixarntz
Copy link
Collaborator

felixarntz left a comment

Looks good to me except for two minor things to clarify.

@@ -22,6 +22,7 @@ install:
- nvm install 8.11.4 && nvm use 8.11.4
- export DEV_LIB_PATH=dev-lib
- source $DEV_LIB_PATH/travis.install.sh
- npx grunt shell:webpack_production

This comment has been minimized.

Copy link
@felixarntz

felixarntz Dec 27, 2018

Collaborator

Did a change happen in the build process, or why is this in here now?

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 27, 2018

Author Member

We never did a build of the JS before becuase the tests haven't actually run the JS yet. That needs to change in the future, but that is why it hasn't been an issue thus far.

</div>
<?php
}
if ( ! file_exists( __DIR__ . '/vendor/autoload.php' ) || ! file_exists( __DIR__ . '/vendor/sabberworm/php-css-parser' ) ) {
add_action( 'admin_notices', '_amp_print_composer_install_admin_notice' );
if ( ! file_exists( __DIR__ . '/vendor/autoload.php' ) || ! file_exists( __DIR__ . '/vendor/sabberworm/php-css-parser' ) || ! file_exists( __DIR__ . '/assets/js/amp-block-editor-toggle-compiled.js' ) ) {

This comment has been minimized.

Copy link
@felixarntz

felixarntz Dec 27, 2018

Collaborator

Do we need the second check here? The first should already indicate composer install hasn't run already.

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 27, 2018

Author Member

The second conditional? It's not strictly needed, but it helps ensure that composer installation completed.

This comment has been minimized.

Copy link
@felixarntz

felixarntz Dec 27, 2018

Collaborator

Tbh I'm not sure how one file would be there while the other isn't. But just to make sure, let's keep this. If we add more dependencies in the future, I'd rather re-evaluate and only check for vendor/autoload.php then.

@westonruter westonruter merged commit 506312f into 1.0 Dec 27, 2018

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/npm-run-build-check branch Dec 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.