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

Update WordPress Packages #347

Closed
wants to merge 8 commits into from
Closed

Conversation

ellatrix
Copy link
Member

Trac ticket: https://core.trac.wordpress.org/ticket/50420


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@gziolo
Copy link
Member

gziolo commented Jun 25, 2020

We need to update node and npm versions according to https://make.wordpress.org/systems/2020/01/21/update-svn1-node-versions-for-gutenberg/:

# node --version
v12.16.3
# npm --version
6.14.4

@ellatrix
Copy link
Member Author

@ntwb @pento Do you have any idea what we should use?

@gziolo
Copy link
Member

gziolo commented Jun 25, 2020

function gutenberg_enqueue_block_editor_assets_block_directory() {
 		wp_enqueue_script( 'wp-block-directory' );
		wp_enqueue_style( 'wp-block-directory' );
}
add_action( 'enqueue_block_editor_assets', 'gutenberg_enqueue_block_editor_assets_block_directory' );

All that is necessary to enable Block Directory.

@gziolo
Copy link
Member

gziolo commented Jun 25, 2020

Travis works much better with the latest npm but there are some failure in a different place:

https://travis-ci.com/github/WordPress/wordpress-develop/jobs/353584741#L2553

Running "qunit:files" (qunit) task
2554Testing tests/qunit/compiled.html .........................................................................................................................................................................................................F.............................................................................................
2555>> Customizer Model Utility functions - global failure
2556>> Message: Uncaught TypeError: Cannot read property '__' of undefined
2557>> Actual: null
2558>> Expected: undefined
2559>> TypeError: Cannot read property '__' of undefined
2560>>   at file:///home/travis/build/WordPress/wordpress-develop/build/wp-includes/js/dist/a11y.min.js:2:2033
2561>>   at HTMLDocument.p (file:///home/travis/build/WordPress/wordpress-develop/build/wp-includes/js/dist/a11y.min.js:2:2360)
2562
2563Testing tests/qunit/index.html Access to XMLHttpRequest at 'file:///wp-admin/admin-ajax.php' from origin 'null' has been blocked by CORS policy: Cross origin requests are only supported for protocol schemes: http, data, chrome, https.
2564Failed to load resource: net::ERR_FAILED
2565.........................................................................................................................................................................................................F.............................................................................................
2566>> Customizer Model Utility functions - global failure
2567>> Message: Uncaught TypeError: Cannot read property '__' of undefined
2568>> Actual: null
2569>> Expected: undefined
2570>> TypeError: Cannot read property '__' of undefined
2571>>   at addIntroText (file:///home/travis/build/WordPress/wordpress-develop/build/wp-includes/js/dist/a11y.js:135:56)
2572>>   at HTMLDocument.setup (file:///home/travis/build/WordPress/wordpress-develop/build/wp-includes/js/dist/a11y.js:250:5)
2573
2574Warning: 590 tests completed with 2 failed, 0 skipped, and 0 todo. 
25751740 assertions (in 9179ms), passed: 1738, failed: 2� Use --force to continue.
2576
2577Aborted due to warnings.

@TimothyBJacobs
Copy link
Member

We're tackling that block directory PHP in #359.

@ellatrix
Copy link
Member Author

ellatrix commented Jun 26, 2020

There was 1 failure:
2847
28481) Tests_Dependencies_Scripts::test_wp_add_inline_script_before_after_concat_with_core_dependency
2849Failed asserting that two strings are equal.
2850--- Expected
2851+++ Actual
2852@@ @@
2853 ( 'fetch' in window ) || document.write( '<script src="http://example.org/wp-includes/js/dist/vendor/wp-polyfill-fetch.min.js"></scr' + 'ipt>' );( document.contains ) || document.write( '<script src="http://example.org/wp-includes/js/dist/vendor/wp-polyfill-node-contains.min.js"></scr' + 'ipt>' );( window.DOMRect ) || document.write( '<script src="http://example.org/wp-includes/js/dist/vendor/wp-polyfill-dom-rect.min.js"></scr' + 'ipt>' );( window.URL && window.URL.prototype && window.URLSearchParams ) || document.write( '<script src="http://example.org/wp-includes/js/dist/vendor/wp-polyfill-url.min.js"></scr' + 'ipt>' );( window.FormData && window.FormData.prototype.keys ) || document.write( '<script src="http://example.org/wp-includes/js/dist/vendor/wp-polyfill-formdata.min.js"></scr' + 'ipt>' );( Element.prototype.matches && Element.prototype.closest ) || document.write( '<script src="http://example.org/wp-includes/js/dist/vendor/wp-polyfill-element-closest.min.js"></scr' + 'ipt>' );\n
2854 </script>\n
2855 <script type='text/javascript' src='/wp-includes/js/dist/dom-ready.min.js'></script>\n
2856+<script type='text/javascript' src='/wp-includes/js/dist/i18n.min.js'></script>\n
2857+<script type='text/javascript'>\n
2858+( function( domain, translations ) {\n
2859+	var localeData = translations.locale_data[ domain ] || translations.locale_data.messages;\n
2860+	localeData[""].domain = domain;\n
2861+	wp.i18n.setLocaleData( localeData, domain );\n
2862+} )( "default", { "locale_data": { "messages": { "": {} } } } );\n
2863+</script>\n
2864 <script type='text/javascript' src='/wp-includes/js/dist/a11y.min.js'></script>\n
2865 <script type='text/javascript' src='http://example2.com'></script>\n
2866 <script type='text/javascript'>\n

@ellatrix
Copy link
Member Author

For the e2e tests: Error: Cannot find module 'puppeteer'
Looking into that now.

@@ -76,7 +76,7 @@ before_script:
fi
- npm --version
- node --version
- nvm install 10.13.0
- nvm install --latest-npm
Copy link

@ntsekouras ntsekouras Jun 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm installs nodejs and you removed it to update npm.

nvm install 10.13.0 I think it should stay as before (or use newer node.. I don't know the requirements ).

nvm install --latest-npm tries to update npm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, isn't this what Gutenberg does?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking into Travis CI, as I haven't used it before, but it's definitely a different command, that does the above. Maybe node is already installed or installed in some other place.

Since it was there in WordPress Travis, it should stay there, except some change in node installation has happened.

Also the command --latest-npm is in different Travis hooks:
Wordpress: before_script
GB: before_install

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With nodejs installation comes the npm too.. So it might explain the Puppeteer not found module... ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot(1)

So it actually installs new nodejs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Puppeteer isn’t installed then it means that the lock file wasn’t properly updated. The simplest way to fix it is to install the alias that wp-scripts uses as a dev dependency for the project, example:

https://github.com/WordPress/gutenberg/blob/7060b11b9b9efc00e099c9092fe520402d77ee4b/package.json#L180

It’s a bug in npm’s implementation.

Copy link
Member Author

@ellatrix ellatrix Jun 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're still getting this error:

Downloading Chromium r737027 - 118.4 Mb [====================] 100% 0.0s 
Chromium (737027) downloaded to /Applications/MAMP/htdocs/wordpress-git/node_modules/@wordpress/scripts/node_modules/puppeteer/.local-chromium/mac-737027
Error: Could not find browser revision 737027. Run "npm install" or "yarn install" to download a browser binary.
  at ChromeLauncher.launch (/Applications/MAMP/htdocs/wordpress-git/node_modules/puppeteer/lib/Launcher.js:200:23)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Chromium is downloaded to node_modules/@wordpress/scripts/node_modules/puppeteer, while puppeteer runs from node_modules/puppeteer.

@gziolo
Copy link
Member

gziolo commented Jun 26, 2020

Great work team!!! 🎉

@ellatrix ellatrix closed this Jun 26, 2020
@ellatrix ellatrix deleted the test/50420 branch June 26, 2020 18:10
dmallory42 added a commit to Automattic/woocommerce-payments that referenced this pull request Jan 26, 2021
DanReyLop pushed a commit to Automattic/woocommerce-payments that referenced this pull request Feb 2, 2021
* Enqueue Sift JS on every WCPay page

Fixes server issue 472

For more context and instructions, see the server PR.

* Fix lint

* Adding ActionScheduler dependency to composer.json

* Update to include the Action Scheduler service and load actions on init.

* Added logic to schedule the action when an order is created

* Made WC_Payments_Action_Scheduler_Service non-static as it will need to support calls to the Payment Server API.

* Updated the action names to try and make it a bit more clear

* Added API client logic to make the request

* small updates to naming based on Luiz's suggestions.

* Update to create wrapper function for AS scheduling to allow easier mocking.

* Update to make the tests match the update to add ActionScheduler service to Payment Gateway.

* Fix for a comparison issue, adding tests.

* Fix for a bug where we were getting payment method title rather than payment method.

* Updated the unit tests.

* Client updates after discussion with Luiz.

- Adjust the logic to only fire the creation event when we have sufficient data (intent ID).
- Sets a metadata property to show when an order has been tracked.
- Updates to changelog.

* Updated unit tests

* Add puppeteer dependency and rebuild package.lock to fix E2E tests

For more info, please see: WordPress/wordpress-develop#347 (comment)

* Added `eslint-plugin-import` to stop JS lint test failing.

* trying to fix issues with js linting

* trying to fix issue with e2e tests

* Trying to fix e2e tests

* Updating the linter config temporarily until issues with eslint-plugin integration are worked out

* Fixed an out of date snapshot on jest tests

* reverting package.json and package-lock.json to how they are in .

* Revert `index.js.snap`

* Revert package-lock.json to how it is in trunk.

* Updates based on discussion with Luiz to support update logic (not implemented in server yet.

* Added actionscheduler to dev dependencies.

* Add session ID to the create customer request

We're sending the session_id on the create_customer
WCPay call, so the server can link the newly created
customer with the session.

* Update WCPay gateway to check sift is enabled before scheduling order tracking.

* Mock WC_Payments_Account in WC_Payment_Gateway_WCPay_Test

* Fix unit tests

* Adding test to cover new behaviour

* All the changes we discussed for correctly linking session with user. Brain is too fried for a decent commit message.

* Fix linting errors

Co-authored-by: Daniel Mallory <daniel.mallory@automattic.com>
Co-authored-by: Luiz Reis <luiz.reis@automattic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants