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 compatibility with txiki.js #1895

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Conversation

EmixamPP
Copy link
Contributor

@EmixamPP EmixamPP commented Jun 27, 2024

txiki.js is a small JavaScript runtime compatible with most of the browser api; very nice for small embedded devices.

However, MQTT.js wrongly assume a Node.js compatibility because there is no document variable in the global space. Although, if it was using the browser ws, the lib would be compatible with it.

After discussion saghul/txiki.js#557 with the creator of txiki.js, we both think that it should be the job to add the compatibility.

That's what I've added in the present PR.

Small remark, I also think that, in the future, it may be good to revisit the way MQTT.js determine if it can use native websocket or Node.js one through the ws module. Like checking if WebSocket exists in the global space, instead of trying to determine the runtime name or whatsoever sophisticated tricks.

@robertsLando
Copy link
Member

Like checking if WebSocket exists in the global space, instead of trying to determine the runtime name or whatsoever sophisticated tricks.

Problem is that NodeJS is introducing Websocket support so when that will happen that check would wrongly assume the env to be node and not browser. I have thought many times how this could be improved but I never found a better way to do this, if you find one please tell me or open a PR :)

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

Please fix lint issue npm run lint-fix

@EmixamPP
Copy link
Contributor Author

Oops I would need to git force, I committed with my wrong email address.

txiki.js is compatible with most of the browser api, but it does not have the document variable.
@EmixamPP
Copy link
Contributor Author

I have thought many times how this could be improved but I never found a better way to do this, if you find one please tell me or open a PR :)

What the maintainer of txiki.js propose:

AFAICT navigator.userAgent is the right way to test for this, as recommended by the WinterCG: https://common-min-api.proposal.wintercg.org/

More specifically: https://common-min-api.proposal.wintercg.org/#navigator-useragent-requirements

txikijs, Bun and Deno set it. Node doesn't (currently). Node does as of version 21.

auto-merge was automatically disabled June 27, 2024 12:25

Head branch was pushed to by a user without write access

@robertsLando
Copy link
Member

@EmixamPP Thanks for your link, problem is that I think we need to postpone this till all our envs supports navigator.userAgent

@robertsLando robertsLando enabled auto-merge (squash) June 27, 2024 12:38
@EmixamPP
Copy link
Contributor Author

Sorry for the confusion, but is merging the current solution to support txiki.js is ok? Or it should also be postponed because it uses navigator.userAgent (although this shouldn't impact other envs)?

@robertsLando robertsLando merged commit 37b08c9 into mqttjs:main Jun 27, 2024
5 checks passed
@robertsLando
Copy link
Member

@EmixamPP It is ok, what I meant is just that method cannot be used (yet) as I don't think it would support ALL our cases and old frameworks version that may don't have navigator support

@EmixamPP
Copy link
Contributor Author

Thanks for your extremely fast review and merge!

EmixamPP added a commit to EmixamPP/MQTT.js that referenced this pull request Jul 25, 2024
This reverts commit 37b08c9.
Not special support for txiki.js is required thanks to forceNativeWebSocket client option
robertsLando pushed a commit that referenced this pull request Jul 26, 2024
* Revert "fix(browser): add `navigator` polifilly for wechat mini (#1796)"

This reverts commit c26908a.
since the polyfill hardcode navigator, it is impossible to determine the userAgent at runtime

* doc(README): update WeChat instructions

* feat: add forceNativeWebSocket client option

* Revert "feat: add compatibility with txiki.js (#1895)"

This reverts commit 37b08c9.
Not special support for txiki.js is required thanks to forceNativeWebSocket client option

* style: unify import name IS_BROWSER -> isBrowser

* fixup! feat: add forceNativeWebSocket client option

style: fix lint

* fixup! fixup! feat: add forceNativeWebSocket client option

chore: remove test_store folder pushed
refactor: load protocols only once
refactor: use forceNativeWebSocket only for ws choice
doc(README): typo + update forceNativeWebSocket behaviour description
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.

None yet

2 participants