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

querystring.stringify appends =& to params without value #9500

Closed
thisconnect opened this issue Nov 7, 2016 · 4 comments
Closed

querystring.stringify appends =& to params without value #9500

thisconnect opened this issue Nov 7, 2016 · 4 comments
Labels
feature request Issues that request new features to be added to Node.js. querystring Issues and PRs related to the built-in querystring module.

Comments

@thisconnect
Copy link

  • Version: v6.9.1
  • Platform: Darwin 16.0.0 Darwin Kernel Version 16.0.0: Mon Aug 29 17:56:20 PDT 2016; root:xnu-3789.1.32~3/RELEASE_X86_64 x86_64

Assuming this should return 'foo&bar&baz':

const qs = require('querystring')

console.log(qs.stringify(qs.parse('foo&bar&baz')))

acutal output is foo=&bar=&baz=

qs.parse('foo&bar&baz') returns { foo: '', bar: '', baz: '' },
qs.stringify({ foo: '', bar: '', baz: '' }) returns foo=&bar=&baz=

Would you consider this behavior a bug?

related docs https://nodejs.org/api/querystring.html#querystring_querystring_stringify_obj_sep_eq_options

this seems to be the reason that an equal sign is appended to valueless qs params. See following example:

const url = require('url')
const qs = require('querystring')


const href = 'http://example.com/?foo&bar#baz'
const { protocol, host, pathname, query, hash } = url.parse(href)
console.log(url.format({ protocol, host, pathname, query: qs.parse(query), hash }))

// expected http://example.com/?foo&bar#baz
// returned http://example.com/?foo=&bar=#baz

expect to log http://example.com/?foo&bar#baz
actual output http://example.com/?foo=&bar=#baz

Side note qs.stringify doesn't care if the value is '' or null or undefined
console.log(qs.stringify({ foo: '', bar: null, baz: undefined })) returns foo=&bar=&baz=

Personally it would be really helpful if querystring.parse would return null instead of '' and if querystring.stringify would omit the = sign (eq option) when a parameter is null

@ChALkeR ChALkeR added the querystring Issues and PRs related to the built-in querystring module. label Nov 7, 2016
@mscdex mscdex added the feature request Issues that request new features to be added to Node.js. label Nov 7, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Nov 7, 2016

This has been brought up multiple times in the past (including by me). If you need this behavior, I think your best bet is to use an alternative from userland.

@thisconnect
Copy link
Author

@cjihrig it is fixed it in my code. Nevertheless...

Could you point me to the issue that you are referring to?

I can only find:

So do you consider the current behavior a bug that needs to be fixed. OR the desired behavior that should be documented?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 7, 2016

@thisconnect there is also nodejs/node-v0.x-archive#8829 and nodejs/node-v0.x-archive#8825.

I don't necessarily think that the behavior is a bug, but it is confusing IMO. I think it might be too late to do anything about it though, as it would likely break code.

@TimothyGu
Copy link
Member

The current behavior is actually the standardized one. For example, URLSearchParams in both Node.js and the browser acts the same way:

> new URLSearchParams('foo&bar&baz').toString()
< "foo=&bar=&baz="

Plus as @cjihrig pointed out it's too late to change anyway. Thus closing.

bendemboski added a commit to adopted-ember-addons/ember-electron that referenced this issue Nov 21, 2019
nodejs/node#9500 was causing the hidepassed query param to not work because of the URL parsing we do in the test runner. So rather than working around it by changing to hidepassed=1 in testem-electron.js, let's fix it directly in test-runner.js.
RobbieTheWagner pushed a commit to adopted-ember-addons/ember-electron that referenced this issue Nov 24, 2019
#418)

* chore: update yarn.lock

Forgot to commit yarn.lock when removing conventional-changelog-eslint

* chore: Remove unneeded dependencies

* chore: Put ember-cli in peerDependencies

* feat: Update to electron-forge 6

* feat: simplify build pipeline

This commit significantly simplifies the build pipeline. The lynch pin was getting rid of the resources folders, since those were what created a difference between the Electron app as it existed in the source tree, and as it existed when we invoked electron-forge.

Removing that allowed us to put a proper electron-forge project inside the Ember app, complete with its own package.json and node_modules. Then the ember electron commands just need to build the ember app, put the results in the Electron app, and we have a complete electron app exactly as electron-forge would expect.

This also allows us to keep the Electron dependencies separate from the Ember dependencies, which makes dependency management/maintenance much simpler.

* test: Get tests working

* feat: Enable live reload server

Switch for the home-grown reload logic to using Ember's live reload server, which works more reliably and also supports hot reload of CSS

* chore: Improve blueprint messaging

If a file/directory exists at the same path where we want to create the electron forge project when running the blueprint, error gracefully.

If we see that the `ember-electron directory exists, print out a message about upgrading from v2.

* chore: Update .travis.yml from blueprint

Instead of overwriting .travis.yml with a blueprint file (which whacks any customizations the user has made, and requires us to keep our blueprint .travis.yml in sync with the default version as well as worrying about yarn vs npm), use js-yaml to inject the xvfb bits.

* fix: Fix linting

Use the blueprint to update .eslintignore to exclude the electron project's node_modules and our various built output directories

* feat: Build for production by default when making/packaging

`ember electron:make` and `ember electron:package` are much more similar to `ember electron:deploy` (which defaults to building for production) than they are to `ember electron:build` (which defaults to building for development). So this switches the default environment for those commands to `production`.

Making or packaging is much more similar to

* docs: Documentation!

* chore: Remove semantic release

We want better manual control of releases, especially while updating from 2.x to 3.x

* Update appveyor config

Get us on the same node version (10) as Travis, and improve the npm caching slightly

* fix: better solution for hidepassed

nodejs/node#9500 was causing the hidepassed query param to not work because of the URL parsing we do in the test runner. So rather than working around it by changing to hidepassed=1 in testem-electron.js, let's fix it directly in test-runner.js.

* chore: Remove windows query string workaround

It's apparently not needed anymore since we aren't going though all the hacky Electron wrapper scripts and stuff with electron-forge v6

* chore: Prevent redundant appveyor build

* Remove denodeify

Replace it with Node's built in promisify

* Run CI on Node 8

I realized that since we support down to Node 8 and there are a number of relevant differences between it and Node 10 (e.g. promise-based filesystem APIs), we should test against Node 8 so we don't accidentally break compatibility.

* Build available options based on base command for inherited commands

The electron:test and electron:build commands are sub-classes of the built-in test and build commands, so let's build our options off of their options rather than just duplicating them all.

* Update documentation language

* Linting, dependencies, and output dirs/files

Rather than trying to cobble together a decent story for linting an Electron project inside an Ember project, when the Electron tooling doesn't even really have a linting story, let's just disable it in the Electron project. electron-forge by default doesn't configure the project to lint, so let's leave it to users to do their selves and/or for some future time when we have more real-world feedback and/or electron-forge has a linting story.

Make the blueprint also update .travis.yml to install dependencies in the Electron project along with Ember dependencies.

Stop customizing the output directory, and let it stay where electron-forge wants to put it by default. This leaves us more in line with the default tooling, which may make things easier down the road. Also, configure the Electron app's defualt package.json to tell electron-packager to ignore our test and test build directories, so packaged apps don't end up bloated with extra stuff.

* fix: don't assume POSIX path separator

* fixup! fix: don't assume POSIX path separator

Oops, I missed one in the previous commit. If rebasing, please squash this :)
styfle added a commit to vercel/vercel that referenced this issue Aug 10, 2022
Since `vercel dev` performs query string merging with `vercel.json` routes and the requested url, there is sometimes a slight difference between the request that the browser sends and the request that the upstream dev server receives.

Most servers treat the query string key with "empty" value the same as undefined value. Meaning, these two URLs should be equivalent because each has empty values:

- `http://example.com/src/App.vue?vue&type=style&index=0&lang.css`
- `http://example.com/src/App.vue?vue=&type=style&index=0&lang.css=`

However, `vite dev` handles these two URLs differently because of [string comparisons](https://github.com/vitejs/vite/blob/2289d04af5398791c3a01c9e597dc976e593c852/packages/plugin-vue/src/handleHotUpdate.ts#L98-L99) instead of URL parsing, which causes requests from `vercel dev` to fail. Thus, this PR changes from Node.js native URL parsing to a custom implementation in order to handle this corner case and preserve the difference between `?a=` and `?a`.

Ideally this would be [fixed in vite](vitejs/vite#9589), however we might run into other dev servers that also fail in the same way in the future.

- Fixes #7283
- Closes vitejs/vite#9589
- Related to nodejs/node#9500
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. querystring Issues and PRs related to the built-in querystring module.
Projects
None yet
Development

No branches or pull requests

5 participants