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

Vendor common dependencies into the repository #6203

Closed
jackfranklin opened this issue Jul 10, 2020 · 6 comments
Closed

Vendor common dependencies into the repository #6203

jackfranklin opened this issue Jul 10, 2020 · 6 comments
Assignees

Comments

@jackfranklin
Copy link
Collaborator

We are currently working on shipping a browser build. #6202 fixes one of the issues @paullewis found; we need file extensions on our imports to match the ESM spec.

The next challenge is the import of dependencies that we use in any environments. The only dependency we have currently that's common to all environments is Mitt. Right now our bundled code ships as import mitt from 'mitt' but a browser has no idea what mitt refers to and how to resolve it.

importMaps may provide a solution in the future but they are not at a point to be relied on yet.

I'm proposing that we vendor any common dependencies into this repository. This means we can import them as import mitt from './common/third_party/mitt/mitt.es.js (exact path TBC) and that will work in the browser and Node equally well.

The downside is we have to manage these dependencies but I think our list of dependencies will be very small; the vast majority of Pptr's dependencies are for extracting files from zips/DMGs/etc and I think as a rule we'll be very considered about adding new common dependencies in the future. If needed we can introduce some scripts to make managing them and updating them easier (like we have in DevTools-Frontend).

@jackfranklin jackfranklin self-assigned this Jul 10, 2020
@jackfranklin
Copy link
Collaborator Author

@mathiasbynens @paullewis @OrKoN @hanselfmu PTAL :)

@paullewis
Copy link
Contributor

Do you expect this to only apply to mitt, or other dependencies?

I don't know of any other way of doing this besides rolling in a third_party or lib folder, and certainly if the number of deps is low, and it allows the codebase to be fully agnostic I'm personally very supportive.

@hanselfmu
Copy link
Collaborator

It looks like Mitt does not have any dependencies, and seems like a single build can work in both the browser and Node, so this solution seems quite promising, at least for this interim period.

And I agree that we should be careful about adding other common dependencies, because not all the libraries out there are as self-contained as Mitt 😃

@jackfranklin
Copy link
Collaborator Author

Do you expect this to only apply to mitt, or other dependencies?

It would apply to any common dependencies - that is, ones that are used in both the browser and Node version of Puppeteer.

It looks like Mitt does not have any dependencies, and seems like a single build can work in both the browser and Node, so this solution seems quite promising, at least for this interim period.

This is a good point, if Mitt had 5 dependencies, we'd have a bigger challenge on our hands. I hope we can avoid this and keep our dependencies down to a minimum. In our case Mitt being tiny really helps.

As @paullewis says I can't think of another way to do this; I'll put together a proof of concept to see how hard/easy/painful/etc it is.

jackfranklin added a commit that referenced this issue Jul 13, 2020
As discussed in #6203 we need to vendor our common dependencies in so
that when we ship an ESM build all imports point to file paths and do
not rely on Node resolution (e.g. a browser does not understand `import
mitt from 'mitt'`).
jackfranklin added a commit that referenced this issue Jul 13, 2020
As discussed in #6203 we need to vendor our common dependencies in so
that when we ship an ESM build all imports point to file paths and do
not rely on Node resolution (e.g. a browser does not understand `import
mitt from 'mitt'`).
jackfranklin added a commit that referenced this issue Jul 14, 2020
As discussed in #6203 we need to vendor our common dependencies in so
that when we ship an ESM build all imports point to file paths and do
not rely on Node resolution (e.g. a browser does not understand `import
mitt from 'mitt'`).
jackfranklin added a commit that referenced this issue Jul 14, 2020
* chore: vendor Mitt into src/common/third-party

As discussed in #6203 we need to vendor our common dependencies in so
that when we ship an ESM build all imports point to file paths and do
not rely on Node resolution (e.g. a browser does not understand `import
mitt from 'mitt'`).
@stale
Copy link

stale bot commented Jun 26, 2022

We're marking this issue as unconfirmed because it has not had recent activity and we weren't able to confirm it yet. It will be closed if no further activity occurs within the next 30 days.

@stale stale bot added the unconfirmed label Jun 26, 2022
@OrKoN
Copy link
Collaborator

OrKoN commented Jun 26, 2022

I believe this one is fixed.

@OrKoN OrKoN closed this as completed Jun 26, 2022
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

4 participants