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

Fix depth: 0, add depth: false #326

Merged
merged 2 commits into from Aug 16, 2019
Merged

Fix depth: 0, add depth: false #326

merged 2 commits into from Aug 16, 2019

Conversation

idler8
Copy link
Contributor

@idler8 idler8 commented Aug 8, 2019

@ljharb Edit: this PR now adds tests for the current useless behavior of depth 0, and then fixes it. Fixes #261.

//当options.depth为空时,按&符进行分割
assert.deepEqual(qs.parse('ids[0]=1&ids[1]=2&ids[2]=3', { depth: 0 }), { 'ids[0]': 1, 'ids[1]': 2, 'ids[2]': 3 })

测试已经通过
我觉得这可以作为一个补充规则

@ljharb
Copy link
Owner

ljharb commented Aug 8, 2019

Google translate says the title is: "Add a rule for depth==0", and the body:

//When options.depth is empty, split by ampersand
assert.deepEqual(qs.parse('ids[0]=1&ids[1]=2&ids[2]=3', { depth: 0 }), { 'ids[0]': 1, 'ids[1]' : 2, 'ids[2]': 3 })
Test passed
I think this can be used as a supplementary rule.

@ljharb ljharb changed the title 增加一个depth==0时的各式规则 Add a rule for depth==0 ( 增加一个depth==0时的各式规则 ) Aug 8, 2019
Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

The dist folder is generated, so no changes should ever be made in it. Please make the changes inside lib.

Separately, please provide test cases for this new behavior. Is this a bugfix, or a new feature? I can't tell.

@idler8
Copy link
Contributor Author

idler8 commented Aug 8, 2019

hello
fulfill your requirement.
I first used the "pull request" function, not familiar with it.

@idler8
Copy link
Contributor Author

idler8 commented Aug 8, 2019

travis ci构建失败了?
是不是我的错误?
@ljharb

edit: (ljharb): translation added

Did the travis ci build failed?
Is it my mistake?
@ljharb

@idler8
Copy link
Contributor Author

idler8 commented Aug 9, 2019

#246 #261
是否需要这个补充?
@ljharb

edit (ljharb): translation added

#246 #261
Do you need this supplement?
@ljharb

st.deepEqual(qs.parse('a[0]=b&a[1]=c', { depth: 0 }), { 'a[0]': 'b', 'a[1]': 'c' });
st.deepEqual(qs.parse('a[0][0]=b&a[0][1]=c&a[1]=d&e=2', { depth: 0 }), { 'a[0][0]': 'b', 'a[0][1]': 'c', 'a[1]': 'd', e: '2' });
st.end();
});
Copy link
Contributor Author

@idler8 idler8 Aug 9, 2019

Choose a reason for hiding this comment

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

补充了上面的测试
我觉得这是一个新规则

edit (ljharb): translation added

Added the above test
I think this is a new rule.

@idler8 idler8 changed the title Add a rule for depth==0 ( 增加一个depth==0时的各式规则 ) Add a rule for depth==0 ( 增加一个depth==0时的规则 ) Aug 9, 2019
@ljharb
Copy link
Owner

ljharb commented Aug 9, 2019

I think what we need here is to first add a bunch of tests on master with depth: 0 documenting the existing behavior - and then to rebase this PR on top of it, with your change and the resulting test changes.

@idler8
Copy link
Contributor Author

idler8 commented Aug 10, 2019

I am sorry.
I will close this PR because my English is too bad.
I can't understand what I can do for you next.

@idler8 idler8 closed this Aug 10, 2019
@ljharb ljharb reopened this Aug 16, 2019
@ljharb
Copy link
Owner

ljharb commented Aug 16, 2019

I've updated this into two commits; the first that test the current behavior, and the second that make a depth of 0 or false do something intuitive.

I'm split on the semver of this. I could consider it a bugfix, since 0 was surely never intended to act the way it does; i could consider it semver-minor, because it adds depth: false and because it conceptually allows depth to be 0+ instead of 1+; i could consider it semver-major, because someone might be relying on the super weird and useless current behavior of depth: 0.

@ljharb ljharb changed the title Add a rule for depth==0 ( 增加一个depth==0时的规则 ) Fix depth: 0, add depth: false Aug 16, 2019
@ljharb
Copy link
Owner

ljharb commented Aug 16, 2019

Since I'm adding depth: false, this isn't a patch.

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I'm going to go with this one being semver-minor.

If, in fact, someone has found a use case for the current depth: 0 behavior and this breaks them, I may revert that bugfix in the future - but depth: false will remain regardless.

@idler8
Copy link
Contributor Author

idler8 commented Aug 21, 2019

const Req = {
	defaults: {
		baseUrl: '',
		headers: {},
		qsOptions: { arrayFormat: 'indices', encodeValuesOnly: true },
	},
	base(method, url, data, header, extend) {
		return new Promise((success, fail) => {
			wx.request(Object.assign({ method, header, url, data, success, fail }, extend));
		});
	},
	post(url, data) {
		url = this.defaults.baseUrl + url;
		data = qs.parse(qs.stringify(data, this.defaults.qsOptions), { depth: 0 });
		let header = this.defaults.headers;
		return this.base('post', url, data, header).then(res => {
			if (res.statusCode == 200) {
				if (res.data.code == 200) return res.data.data;
				return Promise.reject(res.data.message);
			}
			return Promise.reject('接口请求失败');
		});
	},
	get(url, data) {
		url = this.defaults.baseUrl + url;
		data = qs.parse(qs.stringify(data, this.defaults.qsOptions), { depth: 0 });
		let header = this.defaults.headers;
		return this.base('get', url, data, header).then(res => {
			if (res.statusCode == 200) {
				if (res.data.code == 200) return res.data.data;
				return Promise.reject(res.data.message);
			}
			return Promise.reject('接口请求失败');
		});
	},
};
Req.get('/kan/article', { ids:[1,2,3,4,5] })
$ids = $request->input('ids',[]);

I use it in WeChat applet development.
https://developers.weixin.qq.com/minigame/dev/api/network/request/wx.request.html
WeChat's data property is very bad, I need to make him and my PHPLaravel interface more suitable.

@ljharb
Copy link
Owner

ljharb commented Aug 21, 2019

@idler8 can you file a new issue with this?

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.

Feature request: depth: false or parseDepth: false for qs.parse
2 participants