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

Replace injectFile to injectScript and add injectCSS #949

Closed
SamVerschueren opened this issue Oct 4, 2017 · 8 comments
Closed

Replace injectFile to injectScript and add injectCSS #949

SamVerschueren opened this issue Oct 4, 2017 · 8 comments

Comments

@SamVerschueren
Copy link
Contributor

When browsing trough the docs and using the API, I noticed that Puppeteer has an injectFile method. The name might be confusing, because you might think it works for a CSS file as well. I think it would be better to rename this method to injectScript and also add a injectCSS method.

Just an idea though :).

@vsemozhetbyt
Copy link
Contributor

CSS injecting seems to be possible when #947 is landed.

@aslushnikov
Copy link
Contributor

@vsemozhetbyt injectFile injects from file system, whereas addStyleTag adds a url. I'd rather unify the two methods since it's confusing.

Something like:

page.addScriptTag({
  url: '...', // either URL or PATH could be specified
  path: '..',
});

@SamVerschueren
Copy link
Contributor Author

addScriptTag and addStyleTag currently already work with a local file path, that's what the tests do as well. Not sure if it's a good idea though, that's probably the reason why injectFile exists.

If we would go with your solution, an object with url and path, it might be nice to have a content option as well.

page.addScriptTag({
  url: '...',
  path: '..',
  content: `document.write('Hello World')`
});

@SamVerschueren
Copy link
Contributor Author

@aslushnikov Let me know what the preferred path forward is for this. Happy to do a PR to fix this.

@aslushnikov aslushnikov modified the milestone: v1.0 Oct 10, 2017
@aslushnikov
Copy link
Contributor

@aslushnikov Let me know what the preferred path forward is for this. Happy to do a PR to fix this.

@SamVerschueren Your suggestion with url/path/content options sounds good to me:

page.addScriptTag({
  url: '...',
  path: '..',
  content: `document.write('Hello World')`
});

if you have time, I'd be happy to review a PR. (note: please make sure to use sourceURL for the 'content' option of the addScriptTag/addStyleTag methods).

If you're short on time, I'd rather do it myself: this is one of the bugs that we'd like to have closed soon.

@SamVerschueren
Copy link
Contributor Author

SamVerschueren commented Oct 10, 2017

I'll try to work on it today.

Can you elaborate on

make sure to use sourceURL for the 'content' option of the addScriptTag/addStyleTag methods

Or do you mean

make sure to use sourceURL for the path option

@aslushnikov
Copy link
Contributor

Or do you mean

make sure to use sourceURL for the path option

That's right, I meant path option.

@SamVerschueren
Copy link
Contributor Author

PR send!

ithinkihaveacat pushed a commit to ithinkihaveacat/puppeteer that referenced this issue Oct 31, 2017
…peteer#996)

This patch:
- deprecates injectFile as it was confused with the addScriptTag
- accepts an options object in addScriptTag which supports properties url, path and content.
- accepts an options object in addStyleTag which supports properties url, path and content.

Fixes puppeteer#949.

BREAKING CHANGE:
- the addStyleTag/addScriptTag have changed;
- the injectFile was removed in favor of (addStyleTag({path:}).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants