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.

Sign up for GitHub

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

Jump to bottom

Improved feature-detection of chrome.sockets #36

Merged
merged 2 commits into from
May 3, 2019
Merged

Improved feature-detection of chrome.sockets #36

merged 2 commits into from
May 3, 2019

Conversation

lidel
Copy link
Contributor

@lidel lidel commented Apr 26, 2019

What?

This PR adds explicit check if chrome.sockets.tcp and chrome.sockets.tcpServer exist before registering related listeners.

Why?

chrome-net did not check if used chrome.sockets.* APIs actually exist, which caused problems in browser extensions. Extension was unable to maintain the single codebase and do feature-detection at runtime (eg. Google Chrome without chrome.sockets vs Brave with).

This PR makes it possible to include chrome-net in generic build pipeline.

Refs. ipfs/ipfs-companion#664 ipfs/ipfs-companion#716

@lidel
fix: feature detection for Google Chrome
838ffde 
Old version did not check if chrome.sockets.* actually exist.
This caused problems with extensions built for Chromium-based browsers
that tried to maintain the single codebase and do feature-detection
at runtime.

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
jscissr
jscissr requested changes Apr 26, 2019
View reviewed changes
Copy link
Collaborator

@jscissr jscissr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM.

index.js Outdated Show resolved Hide resolved
@jscissr @lidel
refactor: remove redundant check
19817c0 
Co-Authored-By: lidel <lidel@lidel.org>
@jscissr jscissr merged commit 1362648 into feross:master May 3, 2019
@lidel
Copy link
Contributor Author

lidel commented May 6, 2019 •
edited
Loading

@jscissr any chance to publish a new release to npm with this? :)

@jscissr
Copy link
Collaborator

jscissr commented May 6, 2019

The problem is that @feross has enabled branch protection with required pull request review for master, and I can't approve my own changes. At the same time his status is:

Not checking GitHub notifications. Email me for urgent issues.

I could publish to npm, but then I can't push the version update commit to GitHub.

Some options I see are:

  • Email @feross and ask him to disable required review or add another collaborator (but I'm not sure if you can call this urgent)
  • If you @lidel open the PR instead of me, I could approve and merge it.

What do you think?

@lidel
Copy link
Contributor Author

lidel commented May 7, 2019

@jscissr I can help and open a PR, just let me know what should be in it :)

Version bump to 3.3.2 ? Anything else?

chrome-net/package.json

Line 4 in 1362648

"version": "3.3.1",

@jscissr
Copy link
Collaborator

jscissr commented May 11, 2019

You should be able to open a PR here: https://github.com/feross/chrome-net/compare/version-update?expand=1

@lidel lidel mentioned this pull request May 13, 2019
Merged
@lidel lidel deleted the fix/feature-detection-in-regular-chrome branch May 13, 2019 09:43
@lidel lidel restored the fix/feature-detection-in-regular-chrome branch May 13, 2019 09:43
@lidel lidel deleted the fix/feature-detection-in-regular-chrome branch May 13, 2019 10:18
lidel added a commit to ipfs/ipfs-companion that referenced this pull request May 13, 2019
@lidel
chore: switch to chrome-net v3.3.2
b5c2252 
Release includes feross/chrome-net#36
improved feature-detection of chrome.sockets
@lidel lidel mentioned this pull request May 13, 2019
Merged
2 tasks
lidel added a commit to ipfs/ipfs-companion that referenced this pull request Jun 2, 2019
@lidel
chore: switch to chrome-net v3.3.2
fc48e9a 
Release includes feross/chrome-net#36
improved feature-detection of chrome.sockets
lidel added a commit to ipfs/ipfs-companion that referenced this pull request Jun 2, 2019
@lidel
chore: dependency updates (#720)
ea21935 
switch to chrome-net v3.3.2
Release includes feross/chrome-net#36
(improved feature-detection of chrome.sockets)

+ usual dependency updates
@feross
Copy link
Owner

feross commented Jul 23, 2019

Sorry that I missed this message. Were you able to get this issue figured out and a new version published to npm?

@lidel
Copy link
Contributor Author

lidel commented Jul 24, 2019

Yes, its all good: it shipped in v3.3.2

@lidel lidel mentioned this pull request Sep 8, 2019
Merged
@lidel
Copy link
Contributor Author

lidel commented Sep 8, 2019

I created a PR with the same fix for UDP API: feross/chrome-dgram#16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jscissr jscissr jscissr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lidel @jscissr @feross