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.

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

Streamline route/request path handling and split params + body in requests #2361

Merged
merged 1 commit into from May 29, 2019

Conversation

@isiahmeadows
Copy link
Collaborator

commented Jan 22, 2019

Description

See the commit message for what all this entails. Here's a quick summary:

  • Split data/useBody into separate params + body for a much easier time using it
  • Drop URL interpolations from query strings
  • Auto-escape URL interpolations, provide :x... for unsafe interpolation
  • Handle query strings better in paths
  • Unify m.request and m.route.set parsing and syntax, break direct dependency on m.buildQueryString/m.parseQueryString.
  • Expose m.buildPathname and m.parsePathname, what each use internally.
  • Support things like :file.:ext and :lang-:region in path names.
  • Document the hell out of everything and update the relevant docs to point to the new page documenting path name syntax.
  • Add a ton of new tests for path testing. (We were testing very little.)
  • Couple drive-by fixes and minor improvements in various places.

Edit: update to include latest stuff.

Motivation and Context

See the commit message after the horizontal line for the context and reasoning behind it.

  • Fixes #2360
  • Fixes #1138
  • Fixes #1788 a little less hackishly
  • Edit: Fixes #2378
  • Probably fixes a few other issues I'm not aware of.

How Has This Been Tested?

Ran all the existing tests, added new tests where necessary, and changed a few existing tests. The changed tests are almost exclusively around m.request/m.jsonp, but I did have to make a minor alteration in the m.route test due to new auto-escaping.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md
@StephanHoyer
Copy link
Member

left a comment

LGTM

Added one additional test for conflicting #/? keys. Don't know what the expected behaviour would be in this case. Maybe we should add a similar test to the parser. Just so it behaves the same, when someone refactors those some time.

Besides that, nice work!

pathname/tests/test-parsePathname.js Show resolved Hide resolved

@isiahmeadows isiahmeadows changed the title Streamline route/request path handling and split params + body in requests WIP: Streamline route/request path handling and split params + body in requests Jan 22, 2019

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 22, 2019

Updated the title to clarify this isn't ready yet. Please don't merge this yet. I'll request new reviews once I'm ready.

I also requested reviews from both of you since it's a pretty significant overhaul, and it's really a part 2 of simplifying the v2 internals outside m.render and fixing all the redundancy around them. This should hopefully be the last major breaking change before v2.0.0.

My other planned changes, all non-breaking and slated for future PRs, are:

  • Semver-patch: Combine the m.mount and m.redraw implementations into a single combined factory and modify the router and request implementations to depend on that instead of just the internal redrawService.
  • Semver-patch: Recast the router to be a single file. 99% of the existing router tests could be easily translated to use the public API, and the remaining 1% are still relatively doable.
  • Semver-minor: Add a third redraw parameter to m.render, make m.render's factory factory return the render function directly, and make m.mount's/m.redraw's factory accept a m.render function.

Everything else I'd like to do that's breaking I'm punting to v3.

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:route-request-path-update branch 2 times, most recently from 75bd78e to 84464cc Jan 22, 2019

@barneycarroll

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Drop URL interpolations from query strings

What's the motivation for this? Interpolating query parameters is surely still a common enough requirement...

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 22, 2019

@barneycarroll Let me clarify: m.route.set("/user/:id", {id: 1, foo: "bar"}) currently sets the route to /user/1?id=1&foo=bar, but after this patch, it would set the route to just /user/1?foo=bar. See #1788 for context (it's a point of confusion), but it's also about not leaving unnecessary junk in the URL. It's also worth noting that some servers reject unknown parameters in their query string, which doesn't play well with just tacking them on like here, hence the second part of my reasoning. This isn't common among servers written in Node, but you might see it in frameworks targeting other languages, and I've personally run into a few third-party APIs that do this.

@barneycarroll

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Gotcha 👍

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 1, 2019

Reminder for later: see if this addresses #2061. Edit: Nope, not fixed.

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:route-request-path-update branch from 84464cc to ef78a8f Feb 2, 2019

@isiahmeadows isiahmeadows changed the title WIP: Streamline route/request path handling and split params + body in requests Streamline route/request path handling and split params + body in requests Feb 2, 2019

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:route-request-path-update branch from ef78a8f to c895bb2 Feb 2, 2019

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 2, 2019

Okay, this is ready for review and should be close to merge quality. I know it's a relatively large PR, but most of the diff is just adding tests and docs where there was nothing previously.

  • Docs: +256, -12
  • Tests: +627, -40
  • Whitespace: +34, -34
  • Everything else: +184, -96
  • Bundle size: 8881 bytes → 9341 Edit: 9289 bytes

In case you're curious, the path names were briefly documented in m.route for declared routes, but only briefly and the syntax for m.request and m.jsonp was almost wholly undocumented.

I know the jump of 400 bytes is a little unsettling, but I feel I could probably regain most, if not all, of that by simply combining api/router.js with router/router.js. There's a lot of duplication going on there, and removing the unnecessary property accesses and function call overhead should help a lot in that area.

/cc @barneycarroll @StephanHoyer

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:route-request-path-update branch 2 times, most recently from 899041a to 26c890e Feb 2, 2019

@isiahmeadows isiahmeadows moved this from Needs triage to High priority in Triage/bugs Feb 2, 2019

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:route-request-path-update branch 2 times, most recently from c5c5698 to a593a3e Feb 7, 2019

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:route-request-path-update branch 2 times, most recently from 67acc62 to 18a2e7a Mar 2, 2019

Streamline route/request path handling, split params + body in requests
Fixes #2360
Fixes #1138
Fixes #1788 a little less hackishly
Probably fixes a few other issues I'm not aware of.

This more or less goes with @lhorie's comment here, just with a minor name
change from `query` to `params`:

#1138 (comment)

Specifically, here's what this patch entails:

- I changed `data` and `useBody` to `params` and `body` in `m.request`.
  Migration is trivial: just use `params` or `body` depending on which you
  intend to send. Most servers do actually care where the data goes, so you can
  generally pretty easily translate this accordingly. If you *really* need the
  old behavior, pass the old value in `params` and if `method === "GET"` or
  `method === "TRACE"`, also in `body`.
- I opened up all methods to have request bodies.
- I fixed `m.parseQueryString` to prefer later values over earlier values and
  to ensure that objects and arrays are persisted across both hash and query
  param parsing. That method also accepts an existing key/value map to append
  to, to simplify deduplication.
- I normalized path interpolation to be identical between routes and requests.
- I no longer include interpolated values in query strings. If you need to
  duplicate values again, rename the interpolation to be a distinct property
  and pass the value you want to duplicate as it.
- I converted `m.route` to use pre-compiled routes instead of its existing
  system of dynamic runtime checking. This shouldn't have a major effect on
  performance short-term, but it'll ease the migration to built-in userland
  components and make it a little easier to reconcile. It'll also come handy
  for large numbers of routes.
- I added support for matching routes like `"/:file.:ext"` or
  `"/:lang-:region"`, giving each defined semantics.
- I added support for matching against routes with static query strings, such
  as `"/edit?type=image": { ... }`.
- I'm throwing a few new informative errors.
- And I've updated the docs accordingly.

I also made a few drive-by edits:

- I fixed a bug in the `Stream.HALT` warning where it warned all but the first
  usage when the intent was to warn only on first use.
- Some of the tests were erroneously using `Stream.HALT` when they should've
  been using `Stream.SKIP`. I've fixed the tests to only test that
  `Stream.HALT === Stream.SKIP` and that it only warns on first use.
- The `m.request` and `m.jsonp` docs signatures were improved to more clearly
  explain how `m.request(url, options?)` and `m.jsonp(url, options?)` translate
  to `m.request(options)` and `m.jsonp(options)` respectively.

-----

There is some justification to these changes:

- In general, it matters surprisingly more than you would expect how things
  translate to HTTP requests. So the comment there suggesting a thing that
  papers over the difference has led to plenty of confusion in both Gitter and
  in GitHub issues.

- A lot of servers expect a GET with a body and no parameters, and leaving
  `m.request` open to working with that makes it much more flexible.

- Sometimes, servers expect a POST with query parameters *instead* of a JSON
  object. I've seen this quite a bit, even with more popular REST APIs like
  Stack Overflow's.

- I've encountered a few servers that expect both parameters and a body, each
  with distinct semantic meaning, so the separation makes it much easier to
  translate into a request.

- Most of the time, path segments are treated individually, and URL-escaping
  the contents is much less error-prone. It also avoids being potentially
  lossy, and when the variable in question isn't trusted, escaping the path
  segment enables you to pass it through the URL and not risk being redirected
  to unexpected locations, avoiding some risks of vulnerabilities and client
  side crashes.

If you really don't care how the template and parameters translate to an
eventual URL, just pass the same object for the `params` and `body` and use
`:param...` for each segment. Either way, the more explicit nature should help
a lot in making the intent clearer, whether you care or not.
@fuzetsu
Copy link
Contributor

left a comment

read through the code/docs (nice work btw)

LGTM 👍

@isiahmeadows isiahmeadows removed the request for review from barneycarroll May 12, 2019

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator Author

commented May 29, 2019

Merging as per private discussion with @StephanHoyer.

@isiahmeadows isiahmeadows merged commit 58f1c74 into MithrilJS:next May 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Triage/bugs automation moved this from High priority to Closed May 29, 2019

@isiahmeadows isiahmeadows deleted the isiahmeadows:route-request-path-update branch May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.