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

[ECO-4121] Publicly document v2 platform compatibility and update CI accordingly #1633

Merged
merged 6 commits into from
Mar 5, 2024

Conversation

VeskeR
Copy link
Contributor

@VeskeR VeskeR commented Feb 26, 2024

Resolves #1605

Changes:

  • Use Node.js 20 in github workflows
  • Run CI tests for node on Node.js 16, 18, 20
  • Update engines in package.json to node >=16
  • Update platform compatibility statements in README and CONTRIBUTING
  • Change build/bundle ES target to ES2017
  • Convert NodeCometTransport in nodejs platform to ES6 classes (otherwise it fails in runtime after building for ES2017)

@github-actions github-actions bot temporarily deployed to staging/pull/1633/features February 26, 2024 13:20 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1633/bundle-report February 26, 2024 13:21 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1633/typedoc February 26, 2024 13:21 Inactive
@VeskeR
Copy link
Contributor Author

VeskeR commented Feb 26, 2024

I'm not sure about this place in README:

Ably-js has fallback mechanisms in order to be able to support older browsers; specifically it supports comet-based connections for browsers that do not support websockets. Each of these fallback transport mechanisms is supported and tested on all the browsers we test against, even when those browsers do not themselves require those fallbacks. These mean that the library should be compatible with nearly any browser on most platforms.
Known browser incompatibilities will be documented as an issue in this repository using the ["compatibility" label](https://github.com/ably/ably-js/issues?q=is%3Aissue+is%3Aopen+label%3A%22compatibility%22).

Should we remove it entirely or reword it somehow? Currently it does imply that we support much older versions than we listed above. At the same time we do still have comet transport in codebase in case websockets doesn't work, even though all of the browser versions we support have websockets API available.

@VeskeR VeskeR marked this pull request as ready for review February 26, 2024 13:29
@VeskeR VeskeR marked this pull request as draft February 26, 2024 14:04
@VeskeR VeskeR linked an issue Feb 26, 2024 that may be closed by this pull request
@github-actions github-actions bot temporarily deployed to staging/pull/1633/features February 26, 2024 15:28 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1633/bundle-report February 26, 2024 15:29 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1633/typedoc February 26, 2024 15:29 Inactive
@VeskeR VeskeR marked this pull request as ready for review February 26, 2024 16:56
@lawrence-forooghian
Copy link
Collaborator

Ably-js has fallback mechanisms in order to be able to support older browsers; specifically it supports comet-based connections for browsers that do not support websockets. Each of these fallback transport mechanisms is supported and tested on all the browsers we test against, even when those browsers do not themselves require those fallbacks. These mean that the library should be compatible with nearly any browser on most platforms.

I think that this statement is misleading. My understanding is that browser compatibility is only one of the reasons that we support alternative (i.e. non-WebSocket) transports. The other is that there are potential network conditions (firewalls or proxies are the ones I’ve seen talked about) that might block WebSocket connections (see the documentation for WebSocketTransport). So, I think we should remove this statement from the compatibility section of the README. Might be good to explain about alternative transports somewhere else, but I don't think it needs to be in this issue.

Known browser incompatibilities will be documented as an issue in this repository using the "compatibility" label.

This statement seems to no longer be accurate — there are no issues using that label and it doesn't appear in our guidance about which labels to use. So I think we can remove that part of the statement.

README.md Outdated Show resolved Hide resolved
@VeskeR
Copy link
Contributor Author

VeskeR commented Mar 1, 2024

The other is that there are potential network conditions (firewalls or proxies are the ones I’ve seen talked about) that might block WebSocket connections (see the documentation for WebSocketTransport).

I didn't know about that, thank you! Now it makes sense why we still need comet transport code even after switching to newer platforms.

Yea, I will reword that paragraph and find a better place for it then. No reason for it to be in the supported platforms section.

This statement seems to no longer be accurate — there are no issues using that label and it doesn't appear in our guidance about which labels to use. So I think we can remove that part of the statement.

Will remove.

@VeskeR
Copy link
Contributor Author

VeskeR commented Mar 1, 2024

Removed fallback transport paragraph from this PR.
Reworded it and added to another section in #1664.

Also applied other changes.

After switching to ES2017 build target in 54d4c39,
ES5 classes in NodeCometTransport were causing "Uncaught TypeError:
Class constructor CometTransport cannot be invoked without 'new'" errors
in CI [1].

[1] https://github.com/ably/ably-js/actions/runs/8049569753/job/21984635358?pr=1633
@VeskeR VeskeR merged commit 7f6f400 into integration/v2 Mar 5, 2024
12 checks passed
@VeskeR VeskeR deleted the 1605/v2-platform-compatibility branch March 5, 2024 20:37
VeskeR added a commit that referenced this pull request May 2, 2024
Also removes comment about old browsers support as it's not needed after
#1633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Publicly document v2 platform compatibility and update CI accordingly
2 participants