WP-API / client-js

Backbone-based JavaScript client for WP API
267 stars 58 forks source link

Correct name route discovery for custom namespaces #152

Closed adamsilverstein closed 7 years ago

adamsilverstein commented 7 years ago

props @westonruter

jazbek commented 7 years ago

@adamsilverstein @westonruter this fixes models, but collections are still broken. Not sure the best way to augment this PR, but here's the line that needs to be fixed for collections:

https://github.com/WP-API/client-js/blob/fix-init-issues/js/load.js#L267

routeModel.get( 'versionString' ) needs to be passed as the 3rd argument to wp.api.utils.extractRoutePart() there.

adamsilverstein commented 7 years ago

@jazbek great, thank you for the testing and detailed feedback!

I'll review and make the fix you suggested. if you want to try augmenting the PR, I think you have to fork the repo, pull the branch, commit your changes, then make a new PR from your repo.

ps. I am working on adding unit tests to core for the JS client in https://core.trac.wordpress.org/ticket/39264 and would like to add a test case for instances like your custom namespace, verifying that the models and collections are constructed as expected. The tests rely on passing 'mocked' schema data, can you provide the schema JSON provided by your site at /wp-json, I can then use this to construct the tests and validate the fixes we have here.

Thanks!

adamsilverstein commented 7 years ago

@jazbek @westonruter I pushed a couple of improvements here:

I started adding tests locally with your JS widgets plugin enabled @westonruter one issue I ran into with my tests is that when a collection returns models with ids, those same ids will work for the single items. this seems like a safe assumptions, however with your endpoints, the id is returned as a slug but the singular endpoint expects the numeric id. you can fix this by accepting the slug at the singular endpoint, or by leaving id as is and changing to something separate for the slug. let me know if this makes sense.

the models and collections from the plugin are now well named:

@jazbek can you test the latest version with these fixes? I pushed a built version to the develop branch if that is helpful.

westonruter commented 7 years ago

I started adding tests locally with your JS widgets plugin enabled @westonruter one issue I ran into with my tests is that when a collection returns models with ids, those same ids will work for the single items. this seems like a safe assumptions, however with your endpoints, the id is returned as a slug but the singular endpoint expects the numeric id. you can fix this by accepting the slug at the singular endpoint, or by leaving id as is and changing to something separate for the slug. let me know if this makes sense.

@adamsilverstein I believe this is fixed in develop now, per https://github.com/xwp/wp-js-widgets/commit/c118a7d7a018baef364c7fb7e1eeaefd7e34d208 and https://github.com/xwp/wp-js-widgets/commit/b5b74fe12f8651c86329b2be36b2d52f3b3170ce

jazbek commented 7 years ago

@adamsilverstein sounds like you got a test schema, but here's mine: https://wordpress.slack.com/files/jessica/F3KAYD8MP/schema_with_custom_namespace.js

Uploaded it to slack the other day and forgot to share it. :)

FWIW, I am localizing the following for the wpApiSettings:

var wpApiSettings = {
    "root": "http:\/\/theatersite.local\/some-location\/wp-json\/",
    "nonce": "[ wp_create_nonce('wp_rest') ]",
    "versionString": "nj\/v1\/",
    "mapping": {
        "models": {
            "NjAuditorium": "Auditorium",
            "NjShowtime": "Showtime",
            "NjShow": "Show",
            "NjTicket_type": "TicketType",
            "NjTicket_type_group": "TicketTypeGroup",
            "NjSeries": "Series"
        },
        "collections": {
            "NjAuditorium": "Auditoriums",
            "NjSeries": "Series",
            "NjShow": "Shows",
            "NjShowtime": "Showtimes",
            "NjTicket_type": "TicketTypes",
            "NjTicket_type_group": "TicketTypeGroups"
        }
    }
};
westonruter commented 7 years ago

@adamsilverstein let me know if I misinterpreted the JS Widgets problem you identified. BTW, the latest code is in the feature/implement-remaining-core-widgets branch, but that should be merged into develop later today.

adamsilverstein commented 7 years ago

@jazbek can you test the latest code in this branch (maybe without your mapping setup) to see if your custom namespaces are parsed correctly now? I wasn't able to download your schema l- the link gives me an error, can you try sharing it again?

adamsilverstein commented 7 years ago

I created a trac ticket to get this change (once finalized) into core: https://core.trac.wordpress.org/ticket/39561

jazbek commented 7 years ago

@adamsilverstein yes, this code seems to work with my setup. i did keep a mapping, because i need some transformations to make our code work (we use camel case in the js instead of underscores for custom post types), but i removed the namespace prefix that the client js library was incorrectly adding.

sorry about the schema, shared the wrong link. here's a gist: https://gist.github.com/jazbek/99074ce1a6f41f4c6ecb4a001d5534fa

westonruter commented 7 years ago

FYI: This has necessitated me to make the following change: https://github.com/xwp/wp-customize-featured-content-demo/pull/7

adamsilverstein commented 7 years ago

@westonruter FYI: I merged this to trunk in https://core.trac.wordpress.org/changeset/40074 and reopened for consideration for 4.7.3