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

Launch options merge fixed #322

Closed
wants to merge 7 commits into from
Closed

Launch options merge fixed #322

wants to merge 7 commits into from

Conversation

zdm
Copy link

@zdm zdm commented Sep 15, 2020

Removed deepmerge when merging options in launch() method.

@zdm zdm mentioned this pull request Sep 15, 2020
@itsdarrylnorris
Copy link
Contributor

Hey @zdm ,

If I am not wrong this PR solves this issue. We are have considering moving to object-merge-advanced with isPlainObject.

Do you know if this will fix your problem?

@zdm
Copy link
Author

zdm commented Sep 15, 2020 via email

@zdm
Copy link
Author

zdm commented Sep 20, 2020

The test options/will modify puppeteer launch options through plugins is not passed even on original code.

Could somebody merge this PR pls?

@Niek Niek requested a review from berstend September 28, 2020 08:14
@berstend
Copy link
Owner

berstend commented Oct 3, 2020

What's the benefit of this, performance?

This will be gone in the rewrite (#303) anyway: https://github.com/berstend/puppeteer-extra/pull/303/files#diff-204794cd3a2e9247e20d8a1f99cc73c8R75-R91

I've pushed the responsibility of checking that options.args exists (and is an array) to the plugins, to make the base code cleaner.

I'm inclined to close this issue until there's more info provided.

@berstend berstend added the more-info-needed Further information is requested label Oct 3, 2020
@berstend
Copy link
Owner

Closing in favor of #268

@berstend berstend closed this Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-info-needed Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants