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

Puppeteer core dependency #225

Merged
merged 6 commits into from
Mar 9, 2024
Merged

Puppeteer core dependency #225

merged 6 commits into from
Mar 9, 2024

Conversation

sbounmy
Copy link
Contributor

@sbounmy sbounmy commented Feb 12, 2024

when using remote chromium browserless/chrome, puppeteer-core is more lightweight and avoid download chrome.

Copy link

@rud rud left a comment

Choose a reason for hiding this comment

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

Sounds like an excellent plan!

An idea: Maybe make it a config option so one can explicitly pick whether puppeteer or puppeteer-core is the command to invoke?

Note: I'm not affiliated with the project, but this seems like a good plan.

@sbounmy
Copy link
Contributor Author

sbounmy commented Mar 3, 2024

@rud IMHO its better to not have another config option.
Most people have puppeteer or puppeteer-core installed so the gem should be ale to pick which one is available.
puppeteer or fallback to puppeteer-core

@sbounmy
Copy link
Contributor Author

sbounmy commented Mar 6, 2024

@abrom any update for merging this ?

Copy link
Contributor

@abrom abrom left a comment

Choose a reason for hiding this comment

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

Nice one @sbounmy looks great! I've flagged a few minor things but I've pushed a commit to address them 😄

README.md Outdated
@@ -194,6 +194,12 @@ File.open("grover.png", "wb") { |f| f << grover.to_png }

You can also pass launch flags like this: `ws://localhost:3000/?--disable-speech-api`

If you are only using remote chromium, you better use `puppeteer-core` instead of `puppeteer` to avoid downloading chrome.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you are only using remote chromium, you better use `puppeteer-core` instead of `puppeteer` to avoid downloading chrome.
If you are only using remote chromium, you can install the `puppeteer-core` node package instead of `puppeteer` to avoid downloading chrome.

var puppeteer = require(require.resolve('puppeteer-core', { paths: Module._nodeModulePaths(process.cwd()) }));
} catch (coreError) {
// raise the original puppeteer load issue so we don't send people don't the wrong rabbit hole by default.
throw puppeteerError;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -296,4 +308,4 @@ require('readline').createInterface({
} catch(error) {
_handleError(error);
}
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer a trailing newline 😄

@@ -95,6 +101,23 @@
end
end

context 'when puppeteer-core package only is installed', :remote_browser do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
context 'when puppeteer-core package only is installed', :remote_browser do
context 'when only the puppeteer-core package is installed', :remote_browser do

@@ -1067,4 +1090,4 @@ def mean_colour_statistics(image)
end
end
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer a trailing newline

Comment on lines 114 to 118
let(:url_or_html) { '<html><body style="background-color: blue"></body></html>' }

it 'does not raise' do
expect { convert }.to_not raise_error
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Specs look good, but I think we could do better with one that also checks that the result actually was a PDF with the expected content. Something like:

Suggested change
let(:url_or_html) { '<html><body style="background-color: blue"></body></html>' }
it 'does not raise' do
expect { convert }.to_not raise_error
end
let(:url_or_html) { '<html><body style="background-color: blue">puppeteer-core works!</body></html>' }
it { expect { convert }.not_to raise_error }
it { expect(pdf_text_content).to eq 'puppeteer-core works!' }

Comment on lines 105 to 110
before do
FileUtils.move 'node_modules/puppeteer', 'node_modules/puppeteer_temp'
end
after do
FileUtils.move 'node_modules/puppeteer_temp', 'node_modules/puppeteer'
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Rubocop is either going to want a newline between these, or use the inline block form:

Suggested change
before do
FileUtils.move 'node_modules/puppeteer', 'node_modules/puppeteer_temp'
end
after do
FileUtils.move 'node_modules/puppeteer_temp', 'node_modules/puppeteer'
end
before { FileUtils.move 'node_modules/puppeteer', 'node_modules/puppeteer_temp' }
after { FileUtils.move 'node_modules/puppeteer_temp', 'node_modules/puppeteer' }

@abrom abrom merged commit 6c64414 into Studiosity:main Mar 9, 2024
12 checks passed
@abrom
Copy link
Contributor

abrom commented Mar 9, 2024

Released in v1.1.7

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.

None yet

3 participants