setHtml() and getHtml() API #112

seangransee · 2017-07-31T19:10:25Z

This implements an API at setHtml() to call Page.setDocumentContent, allowing the user to pass arbitrary HTML to Chrome instead of calling a URL.

It also implements an API at getHtml() that allows the user to get the page's HTML.

Example usage:

const { Chromeless } = require('chromeless')

async function run() {
  const chromeless = new Chromeless()

  const html = await chromeless
    .setHtml('<h1>Hello world!</h1>')
    .getHtml()

  console.log(html)

  await chromeless.end()
}

run().catch(console.error.bind(console))

Running this logs the following content:

<html><head></head><body><h1>Hello world!</h1></body></html>

Another example of getHtml(), on an actual web page:

chromeless
  .goto('http://www.google.com/')
  .getHtml()

joelgriffith · 2017-07-31T19:29:42Z

src/chrome/local-runtime.ts

@@ -114,6 +117,10 @@ export default class LocalRuntime {
    return scrollTo(this.client, x, y)
  }



I'm not sure what the intent of this private handler is for?

I was following the same pattern as similar methods:

private async goto(url: string): Promise<void> { ... } private async click(selector: string): Promise<void> { ... } private async scrollTo<T>(x: number, y: number): Promise<void> { ... } private async setHtml(html: string): Promise<void> { ... } // new method in this PR

joelgriffith · 2017-07-31T19:33:01Z

src/api.ts

@@ -142,6 +142,12 @@ export default class Chromeless<T extends any> implements Promise<T> {
    return this
  }

+  setDocumentContent(html: string): Chromeless<T> {


I'm a bit against naming public methods the same thing as their counterparts in CDP. It (in my head at least) feels like it "locks" us a bit to the implementation details of CPD as opposed to just offering a feature regardless of how it's implemented.

I'm not too strongly tied to this opinion, but wanted to gather folks' thoughts

I definitely see your point. I'm open to other suggestions for names for this method. Perhaps setHTML()?

If we do a setHtml() then.. we need a getHtml(), too?

I actually do think there should be a getHtml() method. That way, the example usage for setHtml() could look more like this:

const html = await chromeless .setHtml('<h1>Hello world!</h1>') .getHtml() console.log(html) // <html><head></head><body><h1>Hello world!</h1></body></html>

I renamed setDocumentContent() to setHtml() in 1f9a300.

My next step is to implement getHtml().

Alright, setHtml() and getHtml() are now implemented.

const html = await chromeless .setHtml('<h1>Hello world!</h1>') .getHtml() console.log(html) // <html><head></head><body><h1>Hello world!</h1></body></html>

seangransee · 2017-07-31T21:01:39Z

Looks like the getHtml() method I implemented here was requested in #74, so that issue can be closed if this PR gets merged.

timsuchanek · 2017-08-01T09:58:51Z

Looks good from my side. If nothing speaks against it, we can merge

adieuadieu

The implementation looks good to me. However, I have two documentation-related requests:

please also update the API TOC in the project's main README
please update the Unreleased section in the CHANGELOG. I think this qualifies as a notable addition. (Please mention this PR setHtml() and getHtml() API #112 and Add .html method #74)

seangransee · 2017-08-01T14:22:05Z

@adieuadieu I have made your requested changes to the two markdown files.

adieuadieu

@seangransee thank you!

joelgriffith · 2017-08-01T15:13:04Z

Thanks @seangransee!

adieuadieu requested review from joelgriffith and timsuchanek July 31, 2017 19:11

adieuadieu added API feature labels Jul 31, 2017

joelgriffith reviewed Jul 31, 2017

View reviewed changes

seangransee force-pushed the set-document-content branch from 1811770 to 0e274ce Compare July 31, 2017 19:51

seangransee changed the title ~~setDocumentContent() API~~ setHtml() API Jul 31, 2017

seangransee changed the title ~~setHtml() API~~ setHtml() and getHtml() API Jul 31, 2017

seangransee mentioned this pull request Jul 31, 2017

Add .html method #74

Closed

vladholubiev approved these changes Jul 31, 2017

View reviewed changes

joelgriffith approved these changes Aug 1, 2017

View reviewed changes

adieuadieu suggested changes Aug 1, 2017

View reviewed changes

seangransee added 7 commits August 1, 2017 09:17

implement setDocumentContent() API

5ddb9fd

rename setDocumentContent() to setHtml()

42f51aa

implement getHtml()

95f4f34

fix docs

dfd446f

rename html() to getHtml()

a9fdcf3

update the API TOC in the project's main README

e1d8b47

add to Unreleased section of CHANGELOG.md

bee325a

seangransee force-pushed the set-document-content branch from cde8587 to bee325a Compare August 1, 2017 14:20

adieuadieu approved these changes Aug 1, 2017

View reviewed changes

adieuadieu merged commit 6c2a4d5 into schickling:master Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

setHtml() and getHtml() API #112

setHtml() and getHtml() API #112

seangransee commented Jul 31, 2017 •

edited

Loading

joelgriffith Jul 31, 2017

seangransee Jul 31, 2017 •

edited

Loading

joelgriffith Jul 31, 2017

seangransee Jul 31, 2017

adieuadieu Jul 31, 2017

seangransee Jul 31, 2017 •

edited

Loading

seangransee Jul 31, 2017

seangransee Jul 31, 2017

seangransee commented Jul 31, 2017

timsuchanek commented Aug 1, 2017

adieuadieu left a comment •

edited

Loading

seangransee commented Aug 1, 2017

adieuadieu left a comment

joelgriffith commented Aug 1, 2017

		@@ -114,6 +117,10 @@ export default class LocalRuntime {
		return scrollTo(this.client, x, y)
		}

setHtml() and getHtml() API #112

setHtml() and getHtml() API #112

Conversation

seangransee commented Jul 31, 2017 • edited Loading

joelgriffith Jul 31, 2017

Choose a reason for hiding this comment

seangransee Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

joelgriffith Jul 31, 2017

Choose a reason for hiding this comment

seangransee Jul 31, 2017

Choose a reason for hiding this comment

adieuadieu Jul 31, 2017

Choose a reason for hiding this comment

seangransee Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

seangransee Jul 31, 2017

Choose a reason for hiding this comment

seangransee Jul 31, 2017

Choose a reason for hiding this comment

seangransee commented Jul 31, 2017

timsuchanek commented Aug 1, 2017

adieuadieu left a comment • edited Loading

Choose a reason for hiding this comment

seangransee commented Aug 1, 2017

adieuadieu left a comment

Choose a reason for hiding this comment

joelgriffith commented Aug 1, 2017

seangransee commented Jul 31, 2017 •

edited

Loading

seangransee Jul 31, 2017 •

edited

Loading

seangransee Jul 31, 2017 •

edited

Loading

adieuadieu left a comment •

edited

Loading