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

chore(agnostification): create web for web only code #6524

Closed
wants to merge 2 commits into from

Conversation

jackfranklin
Copy link
Contributor

This change makes a web folder to house any code that is just for the
browser and won't work in a Node environment.

We have three folders now to divide code up nicely:

  • node
  • web
  • common.

This change makes a `web` folder to house any code that is just for the
browser and won't work in a Node environment.

We have three folders now to divide code up nicely:

- `node`
- `web`
- `common`.
@jackfranklin
Copy link
Contributor Author

One thing I'd welcome opinions on: the two WebSocketTransport classes are almost identical bar the slight difference in how we create the socket (Node's ws lib vs browser) and in the types (one has private ws: NodeWebSocket and the other private ws: WebSocket).

That said, I cannot find a nice way to avoid the duplication in the code that doesn't 1) make it far more complex, 2) lose some type safety. That's why I've left the duplication as I think it's actually best (or least worst) compared to forcing an abstraction.

@mathiasbynens
Copy link
Member

How did you arrive at web and not browser? The latter seems more precise but I might be missing some context.

@jackfranklin
Copy link
Contributor Author

How did you arrive at web and not browser? The latter seems more precise but I might be missing some context.

None really beyond I think we've talked about it as puppeteer-web and I named the entrypoint web.js. I don't have a strong feeling either way on which we should go but we should be consistent.

@google-cla
Copy link

google-cla bot commented Sep 15, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Sep 15, 2021
@jschfflr
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Sep 15, 2021
@jschfflr
Copy link
Contributor

@jackfranklin I updated the pr - let me know if you think it's still relevant.
There's one change in the Node WebSocket that I'm not sure we can also include in the web version:

'User-Agent': `Puppeteer ${pkg.version}`,

Do you know if that would be possible?

@jschfflr
Copy link
Contributor

Closing this for now as discussed offline with @jackfranklin

@jschfflr jschfflr closed this Sep 23, 2021
@jrandolf-2 jrandolf-2 deleted the tidy-up-browser branch May 3, 2022 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants