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

Add "chromeapp" field to package.json #2

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

feross
Copy link

@feross feross commented Aug 8, 2019

I'm attempting to make a defacto standard for specifying Chrome App dependency substitutions using the "chromeapp" field in package.json.

The "chromeapp" field is just like the "browser" field in package.json except it's intended for Chrome Apps instead of a generic browser environment. Bundler tools like browserify or webpack can be configured to look for the "chromeapp" field instead of the "browser" field when doing a build for a Chrome App.

In this specific package, since Chrome Apps can use raw sockets we want to replace require('net') with require('chrome-net').

@jscissr
Copy link
Owner

jscissr commented Aug 10, 2019

I'm not convinced of this. The first problem is that it adds a dependency on chrome-net for all users of this module, even tough not all might need it.

Also, I think this is just the wrong place to do it. The way I did this was:

var builtins = require('browserify/lib/builtins.js')
var serverBuiltins = {}
Object.keys(builtins).forEach(function (key) {
  serverBuiltins[key] = builtins[key]
})

serverBuiltins.net = require.resolve('chrome-net')
serverBuiltins.http = require.resolve('http-node')

var server = browserify({..., builtins: serverBuiltins})

(Since https://blog.chromium.org/2016/08/from-chrome-apps-to-web.html I haven't worked on Chrome Apps, not sure if this still works.)

Which is not that much configuration. You'd still have to configure browserify to use the chromeapp field, so you need configuration anyway.

With the chromeapp approach, you'd have to add the config in all the packages that use net. So you'd have to submit this PR to all packages you want to use which use net.
Even worse, different packages might depend on different versions of chrome-net.

@feross
Copy link
Author

feross commented Sep 11, 2019

@jscissr Thanks for your response. I agree with you that the production dependency on chrome-net is not ideal.

Your approach of just telling browserify to do a total replacement of net would work most of the time. However, I have packages where it's not enough to merely replace net with chrome-net but there are actually packages that we want to exclude or replace specifically for Chrome Apps using all the power of the "browser" field in package.json.

For example, you can see how we swap out a package specifically for chrome apps here: https://github.com/webtorrent/webtorrent/blob/57945797335368bd81b3a6ab5a417f057e56d1ea/package.json#L29

So given that packages may want to make specific substitutions for different environments (Chrome Apps, Electron, React Native, etc.) I still think the browserField feature I added to browserify makes sense. The user can just say e.g. browserify --browserField electron and let each package make it's own decision about how to support this environment.

To address your concern about the production dependency on chrome-net, what if we just add the "chromeapp" key to package.json without the dependency on chrome-net? Then the end-user can install the version of chrome-net that they prefer and versions between different packages won't get out of sync, as you pointed out.

I'm attempting to make a defacto standard for specifying Chrome App dependency substitutions using the "chromeapp" field in `package.json`.

The "chromeapp" field is just like the "browser" field in `package.json` (see spec here: https://github.com/defunctzombie/package-browser-field-spec) except it's intended for Chrome App targets instead of a generic browser target.

Specifically, since Chrome Apps can use raw sockets, we want to replace 'net' and 'dgram' with 'chrome-net' and 'chrome-dgram', respectively.
@feross
Copy link
Author

feross commented Sep 11, 2019

I updated the PR so you can see what I'm now proposing.

@feross
Copy link
Author

feross commented Nov 12, 2020

@jscissr Ping – do you have any thoughts on this PR?

@jscissr
Copy link
Owner

jscissr commented Nov 13, 2020

With your updated PR, I'm not strongly against it anymore.

However, I'm still not convinced that it's the best solution for your problem.
You say that your solution works like the "browser" field, but nobody adds the following to all packages which depend on buffer:

{
"browser": {
    "buffer": "buffer/"
  }
}

Instead, this is defined in this file: https://github.com/browserify/browserify/blob/master/lib/builtins.js
You could then have different versions of builtins.js for Chrome Apps and all the other platforms, e.g. for Chrome Apps:

exports.http = require.resolve('http-node')
exports.net = require.resolve('chrome-net')
// ...

I personally think this approach would be a much better solution. You're defining these replacement in a single place, rather than in the package.jsons of many different projects. Is there some reason why this would not work for you?

Btw, I haven't updated http-node in quite some time. In case you want to do this, it's not a lot of work for http-node itself, the main task was actually to update http-parser-js when I did it.

@feross
Copy link
Author

feross commented Nov 14, 2020

@goto-bus-stop Do you have thoughts on @jscissr's proposed solution? Also – is it easy to point browserify to a custom builtins.js file so we don't have to ship a chromeapp-builtins.js file in browserify itself?

@jscissr
Copy link
Owner

jscissr commented Nov 14, 2020

There's currently no command line argument in browserify for using custom builtins, but you can easily do it when using the API (see my comment above).

@goto-bus-stop
Copy link

goto-bus-stop commented Nov 14, 2020

That seems like a totally good use of builtins. I want to make builtins overrideable with plugins for a different reason, but with that feature you would also be able to do:

browserify -p chrome-app-builtins

@feross
Copy link
Author

feross commented Nov 17, 2020

@goto-bus-stop Great! This seems like the best path forward for supporting alternate shims for different environments. Then we won't need to get every package that uses e.g. net to depend chrome-net, react-native-net, etc. for every possible environment.

Diego Rodríguez Baquero and others added 4 commits January 13, 2022 19:11
# [1.3.0](v1.2.0...v1.3.0) (2022-01-14)

### Bug Fixes

* add semantic release config ([c524325](c524325))

### Features

* webtorrent org, semantic release ([65665fd](65665fd))
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.

5 participants