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.

Sign up for GitHub

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

Jump to bottom

Expose pcs endpoints for js and css #1214

Merged
merged 3 commits into from Nov 11, 2019
Merged

Expose pcs endpoints for js and css #1214

merged 3 commits into from Nov 11, 2019

Conversation

joewalsh
Copy link
Contributor

@joewalsh joewalsh commented Nov 8, 2019 •
edited

Exposes new pcs endpoints, removes now unused pagelib_body_start and pagelib_body_end endpoints

https://phabricator.wikimedia.org/T237745

@joewalsh joewalsh added the API label Nov 8, 2019
d00rman
d00rman requested changes Nov 11, 2019
View reviewed changes
Copy link
Contributor

@d00rman d00rman left a comment

Choose a reason for hiding this comment

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

LGTM, one comment in-lined. Also, are we sure nothing is using the end points being removed (in the apps) ?

v1/css.yaml
@@ -38,6 +38,7 @@ paths:
enum:
- base
- pagelib
- pcs
Copy link
Contributor

Choose a reason for hiding this comment

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

You are removing pagelib from the description, but not from the enum here. It must be one or the other :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed - pagelib is deprecated but needs to remain available for app clients that still point to that endpoint. I added some clarifying comments in the documentation

@joewalsh
Copy link
Contributor Author

joewalsh commented Nov 11, 2019 •
edited

Also, are we sure nothing is using the end points being removed (in the apps) ?

Yes - the pagelib_body_start and pagelib_body_end endpoints were only in use by mobile-html but that changed and the start and end code is now in pcs.

The pagelib endpoints are still in use by the Android app shuold remain available until the Android app switches to mobile-html and enough users upgrade.

d00rman
d00rman requested changes Nov 11, 2019
View reviewed changes
Copy link
Contributor

@d00rman d00rman left a comment

Choose a reason for hiding this comment

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

Needs one more change.

v1/javascript.yaml Outdated
request:
method: get
uri: '{{options.host}}/{domain}/v1/data/javascript/mobile/pagelib_body_end'
uri: '{{options.host}}/{domain}/v1/data/javascript/mobile/pagelib'
Copy link
Contributor

Choose a reason for hiding this comment

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

s/pagelib/{type}/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@d00rman d00rman merged commit 1f355da into master Nov 11, 2019
@d00rman d00rman deleted the pcs_css_js branch November 11, 2019 13:38
@d00rman
Copy link
Contributor

d00rman commented Nov 11, 2019

Should be deployed in the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d00rman d00rman d00rman approved these changes

Assignees
No one assigned
Labels
API
Milestone
No milestone
2 participants
@joewalsh @d00rman