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

feat: add 'browser' field #188

Merged
merged 1 commit into from
Apr 29, 2024
Merged

feat: add 'browser' field #188

merged 1 commit into from
Apr 29, 2024

Conversation

Symbitic
Copy link
Contributor

I'm excited about this one. This adds a browser entry to package.json so that when bundling for browsers, it will automatically use navigator.bluetooth when importing this package.

@thegecko
Copy link
Owner

I'm not sure what the use case for this is. It's general practice to stub out packages in the build system rather than relying on the package to do it

@Apollon77
Copy link
Contributor

Additionally why you would bundle this for browsers? SHould you not use the official navigator.bluetooth there instead of this library which is basically node.js?

@Symbitic
Copy link
Contributor Author

Additionally why you would bundle this for browsers? SHould you not use the official navigator.bluetooth there instead of this library which is basically node.js?

That's the whole point. import { bluetooth } from "webbluetooth" will import the SimpleBLE bindings on Node and navigator.bluetooth in browsers, meaning you don't need to have separate files or packages for Node and browsers.

The specific use case I have in mind is Node-PoweredUp. It has this whole complex abstract adapter class and Noble and WebBluetooth implementations. With this, I will be able to refactor it to just use WebBluetooth directly and still work equally well on Node and browsers.

@thegecko
Copy link
Owner

What build/packaging system do you use for node-poweredup?

@Symbitic
Copy link
Contributor Author

node-poweredup doesn't have a build system besides tsc, but build systems shouldn't matter. Bundling is done at the application-level, not library level. The bundler (esbuild, rollup, webpack, etc) decides which file to use based on package.json. If the browser field is defined, it uses that file if the target is the web browser. Otherwise, it uses the standard Node.js package resolution.

@Symbitic
Copy link
Contributor Author

Just to be clear: there will be no changes for Node.js users. However, bundlers (webpack, vite, parcel, etc) and CDNs (esm.sh, unpkg, etc) will use the navigator.bluetooth wrapper when targeting browsers.

@thegecko
Copy link
Owner

The only other feedback I have is that adding this suggests the whole of this library will work in a browser environment (e.g. the new constructor). Perhaps we should document this isn't the case?

As feedback is relatively minor, I'm happy to merge this now.

@thegecko thegecko merged commit d8b91bb into thegecko:master Apr 29, 2024
7 checks passed
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.

3 participants