Reset requestOptions after form submission is complete #112

2018-01-22T13:33:24-05:00

timtrinidad commented

2018-01-22 13:33:24 -05:00

(Migrated from github.com)

When submitting a form, subsequent requests keep the requestMethod and requestPayloads; this causes the form to "autosubmit" with the previous values when you load it again.

This PR resets the request options so that subsequent non-form requests go back to using the defaults.

When submitting a form, subsequent requests keep the `requestMethod` and `requestPayload`s; this causes the form to "autosubmit" with the previous values when you load it again. This PR resets the request options so that subsequent non-form requests go back to using the defaults.

BehindTheMath commented

2018-01-22 17:40:48 -05:00

(Migrated from github.com)

You're right; there's no reason that needs to persist.

But I think it would be even better not to use this.options at all, and instead create a new options object right away.

You're right; there's no reason that needs to persist. But I think it would be even better not to use `this.options` at all, and instead create a new `options` object right away.

timtrinidad commented

2018-01-22 18:34:43 -05:00

(Migrated from github.com)

@BehindTheMath good call. I've gone ahead and made that change.

BehindTheMath commented

2018-01-22 18:42:40 -05:00

(Migrated from github.com)

Great!

Can you do the same for attach-link.js?

Great! Can you do the same for [attach-link.js](https://github.com/MoOx/pjax/blob/b5c2120d085d2a4d5d6896b9638c14e06c905e45/lib/proto/attach-link.js)?

timtrinidad (Migrated from github.com) reviewed 2018-01-23 13:24:00 -05:00

lib/proto/attach-link.js

timtrinidad (Migrated from github.com) commented

2018-01-23 13:23:59 -05:00

I didn't see anywhere else in the code requestOptions would have been modified from its default of {}, so I moved its instantiation to the top to be consistent with attach-form.js.

I didn't see anywhere else in the code `requestOptions` would have been modified from its default of `{}`, so I moved its instantiation to the top to be consistent with `attach-form.js`.

timtrinidad (Migrated from github.com) reviewed 2018-01-23 13:24:45 -05:00

package.json

timtrinidad (Migrated from github.com) commented

2018-01-23 13:24:45 -05:00

The ** glob wasn't working for me (and it doesn't seem to be working properly on TravisCI either since only 40 tests were run instead of 77), so I changed this to be explicit.

The `**` glob wasn't working for me (and it doesn't seem to be working properly on TravisCI either since only 40 tests were run instead of 77), so I changed this to be explicit.

BehindTheMath (Migrated from github.com) reviewed 2018-01-23 13:27:15 -05:00

package.json

BehindTheMath (Migrated from github.com) commented

2018-01-23 13:27:15 -05:00

This change should not be necessary. The ** glob should include everything under ./tests/, including all subdirectories.

This change should not be necessary. The `**` glob should include everything under `./tests/`, including all subdirectories.

timtrinidad (Migrated from github.com) reviewed 2018-01-23 13:32:08 -05:00

package.json

timtrinidad (Migrated from github.com) commented

2018-01-23 13:32:08 -05:00

I thought so too, but it doesn't seem to be working even on TravisCI:

Before the change, only 40 tests ran: https://travis-ci.org/MoOx/pjax/jobs/332062989
After the change, 77 tests ran: https://travis-ci.org/MoOx/pjax/jobs/332428661

From what I've read, globstar needs to be explicitly enabled so it may not be portable.

I thought so too, but it doesn't seem to be working even on TravisCI: * Before the change, only 40 tests ran: https://travis-ci.org/MoOx/pjax/jobs/332062989 * After the change, 77 tests ran: https://travis-ci.org/MoOx/pjax/jobs/332428661 [From what I've read](https://stackoverflow.com/a/1691202), `globstar` needs to be explicitly enabled so it may not be portable.

MoOx (Migrated from github.com) reviewed 2018-01-23 14:39:07 -05:00

package.json

MoOx (Migrated from github.com) commented

2018-01-23 14:39:07 -05:00

if that can help, glob not wrapped in string can fail. I would recommend you try using "tests": "tape -r ./tests/setup.js \"./tests/**/*.js\"",

if that can help, glob not wrapped in string can fail. I would recommend you try using `"tests": "tape -r ./tests/setup.js \"./tests/**/*.js\"",`

MoOx (Migrated from github.com) reviewed 2018-01-23 14:39:27 -05:00

package.json

MoOx (Migrated from github.com) commented

2018-01-23 14:39:27 -05:00

(can fail depending on the bash used to run the command)

timtrinidad (Migrated from github.com) reviewed 2018-01-23 15:14:30 -05:00

package.json

timtrinidad (Migrated from github.com) commented

2018-01-23 15:14:30 -05:00

Confirmed that wrapping the arg in quotes works on both my local environment (OS X) and TravisCI. Updated.

Confirmed that wrapping the arg in quotes works on both my local environment (OS X) and [TravisCI](https://travis-ci.org/MoOx/pjax/jobs/332473782). Updated.

BehindTheMath (Migrated from github.com) approved these changes 2018-01-23 15:34:55 -05:00

BehindTheMath commented

2018-01-23 15:35:50 -05:00

(Migrated from github.com)

LGTM.

@MoOx Any other issues?

LGTM. @MoOx Any other issues?

MoOx (Migrated from github.com) approved these changes 2018-01-23 15:40:57 -05:00

MoOx (Migrated from github.com) left a comment

LGTM. Generally speaking, don't wait for my input. You are the users here (as I already said, I currently don't use this lib anywhere atm).
I have seen enough the last few days to trust you @BehindTheMath! Do not hesitate to move forward without my approval unless you really want it (I am still watching the project and follow each issue/pr - I may use this lib soon :))

LGTM. Generally speaking, don't wait for my input. You are the users here (as I already said, I currently don't use this lib anywhere atm). I have seen enough the last few days to trust you @BehindTheMath! Do not hesitate to move forward without my approval unless you really want it (I am still watching the project and follow each issue/pr - I may use this lib soon :))

BehindTheMath commented

2018-01-23 15:42:53 -05:00

(Migrated from github.com)

Ok, thank you!

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#112