Skip to content

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

Merged
gziolo merged 9 commits intoWordPress:masterfrom
mmtr:update/remove-jquery-api-fetch
Aug 2, 2018
Merged

Remove jQuery dependency from @wordpress/api-fetch#8311
gziolo merged 9 commits intoWordPress:masterfrom
mmtr:update/remove-jquery-api-fetch

Conversation

@mmtr
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

mmtr commented Jul 30, 2018

cc @gziolo @ehg

@gziolo gziolo added [Package] API fetch /packages/api-fetch [Type] Enhancement A suggestion for improvement. labels Jul 30, 2018
@mmtr mmtr mentioned this pull request Jul 30, 2018
Merged
@mmtr
Copy link
Copy Markdown
Contributor Author

mmtr commented Jul 30, 2018 •
edited
Loading

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
Copy Markdown
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.

youknowriad
youknowriad reviewed Jul 31, 2018
View reviewed changes
lib/client-assets.php Outdated
Copy link
Copy Markdown
Contributor

@youknowriad youknowriad Jul 31, 2018

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
Copy Markdown
Member

@gziolo gziolo Jul 31, 2018

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
Copy Markdown
Contributor Author

@mmtr mmtr Jul 31, 2018 •
edited
Loading

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
Copy Markdown
Contributor

@youknowriad youknowriad Aug 1, 2018

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
Copy Markdown
Member

@gziolo gziolo Aug 1, 2018

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
Copy Markdown
Contributor Author

@mmtr mmtr Aug 1, 2018

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.

gziolo
gziolo requested changes Jul 31, 2018
View reviewed changes
Copy link
Copy Markdown
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! 💯

packages/api-fetch/src/middlewares/nonce.js Outdated
Copy link
Copy Markdown
Member

@gziolo gziolo Jul 31, 2018

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
Copy Markdown
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.

yeah, it makes sense. renamed!

@gziolo gziolo added this to the 3.5 milestone Aug 1, 2018
@gziolo
Copy link
Copy Markdown
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
Copy Markdown
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?

@gwwar gwwar mentioned this pull request Aug 1, 2018
Merged
7 tasks
@gziolo
Copy link
Copy Markdown
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
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowriad youknowriad youknowriad left review comments

@gziolo gziolo gziolo approved these changes

@adamsilverstein adamsilverstein Awaiting requested review from adamsilverstein

Assignees

No one assigned

Labels

[Package] API fetch /packages/api-fetch [Type] Enhancement A suggestion for improvement.

Projects

None yet

Milestone

3.5

Development

Successfully merging this pull request may close these issues.

3 participants

@mmtr @gziolo @youknowriad