visionmedia / page.js

Micro client-side router inspired by the Express router
http://visionmedia.github.com/page.js
7.68k stars 689 forks source link

Proposal about forms #478

Open PaulMaly opened 6 years ago

PaulMaly commented 6 years ago

Hi, @matthewp! I've an idea:

<form method="GET" action="/products">
   <input name="q" type="search" placeholder="Search...">
   <button>Search</button>
</form>

the result of this form submissing will be URL changes:

/products?q=Macbook

So, maybe it's a good idea to provide an option for handling GET-forms by the router? Just like we already handle all links. It could be something like this:

page.start({
   click: true,
   submit: true
});

I can provide a PR for these changes if you don't mind.

matthewp commented 6 years ago

Sounds good to me!

PaulMaly commented 6 years ago

Great! I'll send a PR.

Btw, are you sure that this:

  /**
   * To work properly with the URL
   * history.location generated polyfill in https://github.com/devote/HTML5-History-API
   */

  var location = isWindow && (window.history.location || window.location);
  var isLocation = !!location;

was need to be changed to this:

  /**
   * To work properly with the URL
   * history.location generated polyfill in https://github.com/devote/HTML5-History-API
   */

  var isLocation = hasWindow && !!(window.history.location || window.location);

Seems, that it's not an equivalent change and we've lost HTML5-History-API polyfill support, isn't?

matthewp commented 6 years ago

Why is it not equivalent exactly?

PaulMaly commented 6 years ago

Because the idea was to rewrite local location variable to window.history.location if this polyfill is used. Not just to check that we've location. Correct me if I'm wrong.

kethinov commented 6 years ago

Something similar to this was discussed in https://github.com/visionmedia/page.js/pull/107 and the previous maintainer declined to accept pull requests on the subject. I went ahead and implemented it as a plugin instead: https://github.com/kethinov/page.js-body-parser.js

Though I haven't tested either of my two page.js plugins since this project became actively maintained again, so they may need some maintenance to get working again.

Also for what it's worth if the new maintainers are interested in reconsidering the issue, I still think it would be neat to have this feature included in page.js itself rather than needing a plugin to add full support for handling forms.

PaulMaly commented 6 years ago

@kethinov Hi!

Hm, I believe it's a quite different thing. In this issue, you suggest handling of POST-forms, right? I'm not sure that it's a bad idea or something, but in a difference of GET-forms, POST-form also sends a request body to the server. And I think that body processing isn't business of a router.

GET-forms and it's processing is very simple - they change URL, as well as any regular link does it. I think processing of such things like query-string and request bodies could be taken out in plugins.

kethinov commented 6 years ago

Yeah, that is the correct distinction. Personally I do think handling the body should be in scope of the router (for both POST and GET), but the consensus was against it before, so I'm not gonna be super broken up about it if the consensus is against it again. 😉

PaulMaly commented 6 years ago

Actually, I'm not really against it. )) But seems even query string is not processing in Page.js by default. To stay tiny, I guess. For me, it's not a problem to import qs or something and write 3 loc middleware to support it.

Anyway, your idea deserves attention, so maybe you should raise an issue again to check whether the consensus has changed?

PaulMaly commented 6 years ago

PR: #480