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

Use URLSearchParams for parsing search params #357

Merged
merged 9 commits into from
Apr 10, 2020

Conversation

mohsen1
Copy link
Contributor

@mohsen1 mohsen1 commented Mar 29, 2020

URL search params is a lot more involved than simple split by & and making a a key/value pair from it. I noticed in the router an assumption is made that search params are not repeated. This PR does not fix the issue because this assumption goes deeper in the router. Instead, I've introduced standard URLSearchParasm for parsing the search params which handles a lot of corner cases such as unicode and also allow us to get a list for repeated search arguments if we decided to support that.

I've also updated comments in the util.js to use JSDoc so comments show up in hover in editors.

Also used String.prototype.startsWith instead of path[0] for better readability.


P.S. This is my first PR and you should expect a lot more contribution from me to come! I love how this project is set up and the decisions that are made are almost perfect! ❤️


return [...searchParams.keys()].reduce((params, key) => ({
...params,
[key]: searchParams.get(key)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is possible to do searchParams.getAll(key) instead

@mojombo
Copy link
Contributor

mojombo commented Mar 31, 2020

Great suggestion! I wonder if there's a way to have Babel dictate the inclusion of the polyfill instead of including it directly. We'd like to end up allowing users to specify what browser support they want to satisfy and have all Redwood dependencies be transpiled for that specification, at which point Babel could make the decision of whether this polyfill was needed or not.

@peterp curious what our latest thinking is on this.

@mohsen1
Copy link
Contributor Author

mohsen1 commented Mar 31, 2020

So according to Babel docs for preset-env we can enable useBuiltIns and set it to "usage", then core-js will be pollyfilling things like URLSearchParams according to the browserlist config.

Going to try this out and let you know how it works in practice.

@mohsen1
Copy link
Contributor Author

mohsen1 commented Mar 31, 2020

Ok, if Redwood is compiling router in userland using Babel, it will add the polyfill as it does in compiled version here:

"use strict";

var _interopRequireDefault = require("@babel/runtime-corejs3/helpers/interopRequireDefault");

var _Object$defineProperty = require("@babel/runtime-corejs3/core-js/object/define-property");

_Object$defineProperty(exports, "__esModule", {
  value: true
});

exports.replaceParams = exports.validatePath = exports.parseSearch = exports.matchPath = exports.createNamedContext = void 0;

var _concat = _interopRequireDefault(require("@babel/runtime-corejs3/core-js/instance/concat"));

var _keys = _interopRequireDefault(require("@babel/runtime-corejs3/core-js/object/keys"));

var _forEach = _interopRequireDefault(require("@babel/runtime-corejs3/core-js/instance/for-each"));

var _getIterator2 = _interopRequireDefault(require("@babel/runtime-corejs3/core-js/get-iterator"));

var _startsWith = _interopRequireDefault(require("@babel/runtime-corejs3/core-js/instance/starts-with"));

var _keys2 = _interopRequireDefault(require("@babel/runtime-corejs3/core-js/instance/keys"));

var _urlSearchParams = _interopRequireDefault(require("@babel/runtime-corejs3/core-js/url-search-params"));

var _defineProperty2 = _interopRequireDefault(require("@babel/runtime-corejs3/helpers/defineProperty"));

var _slicedToArray2 = _interopRequireDefault(require("@babel/runtime-corejs3/helpers/slicedToArray"));

var _slice = _interopRequireDefault(require("@babel/runtime-corejs3/core-js/instance/slice"));

@mohsen1 mohsen1 force-pushed the mohsen--router-utils branch from cd0dcd5 to 5321675 Compare March 31, 2020 15:40
@thedavidprice
Copy link
Contributor

thedavidprice commented Apr 1, 2020

Our CI Checks were not running for PRs from external, forked repos. See #358

[EDIT: master merged into PR, which trigger checks. Thanks!]

Thanks in advance!

@thedavidprice thedavidprice requested a review from mojombo April 1, 2020 06:10
Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I just need to test it locally, and write a test to make sure. :)

@peterp peterp merged commit eb1e4c9 into redwoodjs:master Apr 10, 2020
mohsen1 pushed a commit to mohsen1/redwood that referenced this pull request Apr 12, 2020
Use URLSearchParams for parsing search params
@thedavidprice thedavidprice added this to the unassigned-version milestone May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants