Reset requestOptions after form submission is complete #112

Merged

timtrinidad merged 4 commits from master into master 2018-01-23 15:44:27 -05:00

Conversation 14 Commits 4 Files Changed 5

+56 -9

timtrinidad commented 2018-01-22 13:33:24 -05:00 (Migrated from github.com)

Copy link

Copy source

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)

Copy link

Copy source

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)

Copy link

Copy source

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

@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)

Copy link

Copy source

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

Copy link

Copy source

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

Copy link

Copy source

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

Copy link

Copy source

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

Copy link

Copy source

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

Copy link

Copy source

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

Copy link

Copy source

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

(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

Copy link

Copy source

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)

Copy link

Copy source

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

Copy link

Copy source

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)

Copy link

Copy source

Ok, thank you!

Ok, thank you!

Sign in to join this conversation.

Reviewers

No Reviewers

timtrinidad

BehindTheMath

MoOx

Labels

Clear labels

bug

dependencies

Pull requests that update a dependency file

duplicate

enhancement

invalid

question

wontfix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

iLoveElysia

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#112