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

Gracefully handle reused array being passed in #255

Merged
merged 1 commit into from
Jun 20, 2021
Merged

Gracefully handle reused array being passed in #255

merged 1 commit into from
Jun 20, 2021

Conversation

nowifi4u
Copy link
Contributor

@nowifi4u nowifi4u commented Jun 14, 2021

Description: If win32 target gets added to arguments array in config, not a local copy.

Prerequisites: Windows 10 (any version), JavaScript (any supported version), Google Chrome (any version, to run the test below)
Scenario: execute code below
Result: after first call one private tab gets opened, after second call 2 private tabs get opened, after third call 3 private tabs get opened (et cetera)
Expected result: only one tab opened after every call.

Code:

const open = require('open');

function sleep(ms) {
	return new Promise(resolve => setTimeout(resolve, ms));
}

const config = {
	app: {
		name: open.apps.chrome,
		arguments: ['-incognito']
	},
	wait: true
};

(async function() {
	while(true) {
		await open('https://google.com', config);
		await sleep(5000);
	}
}());

@nowifi4u nowifi4u marked this pull request as ready for review June 14, 2021 13:31
@sindresorhus
Copy link
Owner

JavaScript objects and array are reference types and it's up to you to ensure you don't reuse what you pass into methods. This is a common convention. Sure, we can protect about it here, but the problem is your code.

@sindresorhus sindresorhus changed the title Bug: assignment to arguments array Gracefully handle reused array being passed in Jun 20, 2021
@sindresorhus sindresorhus merged commit 492445a into sindresorhus:main Jun 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants