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

Remove jQuery dependency from @wordpress/api-fetch #8311

Merged
merged 9 commits into from
Aug 2, 2018
Merged

Remove jQuery dependency from @wordpress/api-fetch #8311

merged 9 commits into from
Aug 2, 2018

Conversation

mmtr
Copy link
Contributor

@mmtr mmtr commented Jul 30, 2018

This removes the jQuery dependency from the @wordpress/api-fetch package.

The nonce middleware relies now on @wordpress/hooks for listening to the heartbeat ticks events.

@mmtr
Copy link
Contributor Author

mmtr commented Jul 30, 2018

cc @gziolo @ehg

@mmtr
Copy link
Contributor Author

mmtr commented Jul 30, 2018

Not sure why I'm not receiving any output on the test-e2e run

> wp-scripts test-unit-js --config test/e2e/jest.config.json --runInBand
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

@gziolo
Copy link
Member

gziolo commented Jul 31, 2018

We still need to properly update dependencies declared in PHP code. It's not the best experience to be honest. I can help with that.

@@ -167,6 +167,11 @@ function gutenberg_register_scripts_and_styles() {
),
'after'
);
wp_add_inline_script(
'wp-api-fetch',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this addition makes more sense as an addition to the heartbeat script instead of wp-api-fetch

Copy link
Member

Choose a reason for hiding this comment

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

You would have also to add wp-hooks as a dependency of wp-api-fetch because it is now used in the code.

It might be more tricky to put it as part of heartbeat because of wp-hooks dependency.

Copy link
Contributor Author

@mmtr mmtr Jul 31, 2018

Choose a reason for hiding this comment

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

wp-hooks is now a dependency of wp-api-fetch

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think ideally, this script should be attached to heartbeat. It's possible to do so https://wordpress.stackexchange.com/questions/100709/add-a-script-as-a-dependency-to-a-registered-script

This allows us to remove the jQuery dependency from the wp-api-fetch script handle as well. Ideally, we would port heartbeat to a gutenberg package and just do it in the code instead of an inline script.

Anyway, I'm fine with this change if we add a comment explaining what we should do ideally.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good point. @mmtr let us know if you want to iterate or leave it as it is filing an issue to tackle it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @youknowriad for the suggestion! 👍

I have moved the addition to the heartbeat script. Does it look good now?

I'm not very familiar yet with PHP or the WordPress API, so it may be possible I forgot something else. Let me know if that it's the case!

What I have not found is where the jQuery dependency is defined for the wp-api-fetch script. Where is it? It looks like only the wp-edit-post script is defining that dependency.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

There are 2 things to fix and we should be good to go.

Thanks for opening this PR, very anticipated change! 💯

@@ -11,7 +11,7 @@ const createNonceMiddleware = ( nonce ) => ( options, next ) => {
* Configure heartbeat to refresh the wp-api nonce, keeping the editor
* authorization intact.
*/
jQuery( document ).on( 'heartbeat-tick', ( event, response ) => {
addAction( 'heartbeat.tick', 'api-fetch/createNonceMiddleware', ( response ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename api-fetch/createNonceMiddleware to core/api-fetch/create-nonce-middleware? It would make it more similar to other hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it makes sense. renamed!

@gziolo gziolo added this to the 3.5 milestone Aug 1, 2018
@gziolo
Copy link
Member

gziolo commented Aug 1, 2018

package-lock.json needs to be regenerated before we proceed. I would like also to get confirmation from @youknowriad that this solution can be used as is. Should we open a follow up maybe?

@mmtr
Copy link
Contributor Author

mmtr commented Aug 1, 2018

package-lock.json needs to be regenerated before we proceed.

I already did this. Do you miss something?

@gziolo
Copy link
Member

gziolo commented Aug 2, 2018

Looks great, thanks for the last set of improvements 🚢

@gziolo gziolo merged commit b2ec406 into WordPress:master Aug 2, 2018
@mmtr mmtr deleted the update/remove-jquery-api-fetch branch August 2, 2018 22:28
@aduth aduth mentioned this pull request Oct 16, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] API fetch /packages/api-fetch [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants