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-4114] Fix missing ably/react export from ably package on v2 branch #1625

Merged
merged 8 commits into from
Feb 26, 2024

Conversation

VeskeR
Copy link
Contributor

@VeskeR VeskeR commented Feb 15, 2024

This PR is based on #1624 so review that one first.

Fixes missing ably/react export from ably-js NPM package on v2 branch.
Adds a test case to test:package script to confirm that we are able to import from ably/react from ably-js NPM package, mostly based on the guide from playwright Components.

See commit messages for more details.

Resolves #1622

@github-actions github-actions bot temporarily deployed to staging/pull/1625/features February 15, 2024 19:50 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/bundle-report February 15, 2024 19:50 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/typedoc February 15, 2024 19:50 Inactive
@VeskeR VeskeR changed the base branch from main to integration/v2 February 15, 2024 19:51
@github-actions github-actions bot temporarily deployed to staging/pull/1625/features February 15, 2024 19:52 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/bundle-report February 15, 2024 19:53 Inactive
@VeskeR VeskeR changed the base branch from integration/v2 to 1619/fix-ably-import-in-react-hooks February 15, 2024 19:53
@github-actions github-actions bot temporarily deployed to staging/pull/1625/typedoc February 15, 2024 19:53 Inactive
@jamienewcomb jamienewcomb changed the title [SDK-4114] Fix missing ably/react export from ably package on v2 branch [ECO-4114] Fix missing ably/react export from ably package on v2 branch Feb 18, 2024
Also removes "exports" property from package.react.json in react-hooks
folder that is copied to `react` build folder. This file now only has
`"main": "./cjs/index.js"`, which, for not obvious reason to me, needs
to be present in order for `npx attw` check to not show "Resolution
failed" error for node10 for "ably/react" path.

Resolves #1622
@VeskeR VeskeR force-pushed the 1622/fix-ably-react-import branch from 30e755f to 72c131e Compare February 19, 2024 13:31
@github-actions github-actions bot temporarily deployed to staging/pull/1625/features February 19, 2024 13:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/bundle-report February 19, 2024 13:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/typedoc February 19, 2024 13:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/features February 21, 2024 06:17 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/bundle-report February 21, 2024 06:18 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/typedoc February 21, 2024 06:18 Inactive
@VeskeR VeskeR force-pushed the 1622/fix-ably-react-import branch from 01d676e to 241ef0e Compare February 21, 2024 06:22
@github-actions github-actions bot temporarily deployed to staging/pull/1625/features February 21, 2024 06:22 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/bundle-report February 21, 2024 06:23 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/typedoc February 21, 2024 06:23 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/features February 21, 2024 06:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/bundle-report February 21, 2024 06:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/typedoc February 21, 2024 06:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/features February 21, 2024 06:46 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/bundle-report February 21, 2024 06:47 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/typedoc February 21, 2024 06:47 Inactive
…ge/browser/template`

Downgrade to 1.39.0 to match installed `@playwright/test` version.
Otherwise playwright will throw errors on test run due to version
mismatch.
Similar to 9976a84, adds a test for
importing from `ably/react` from ably-js NPM package. This test will
catch NPM package errors like in #1622.

Followed playwright for Components guide [1] to setup this.

[1] https://playwright.dev/docs/test-components
Had to downgrade version for explicit dev dependency for `esbuild` to
match version used by `@playwright/experimental-ct-react` (0.18.20).
It was causing 'Error: Expected "0.19.5" but got "0.18.20"' errors
in CI, see job log [1].

[1] https://github.com/ably/ably-js/actions/runs/7984893618/job/21802628829
@VeskeR VeskeR force-pushed the 1622/fix-ably-react-import branch from 4be7291 to 86ee859 Compare February 21, 2024 07:00
@github-actions github-actions bot temporarily deployed to staging/pull/1625/features February 21, 2024 07:00 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/typedoc February 21, 2024 07:01 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/bundle-report February 21, 2024 07:01 Inactive
@VeskeR VeskeR marked this pull request as ready for review February 21, 2024 11:18
`npm run test-ct` was failing in CI (see job log [1]) with reference
to a npm bug [2] and suggested to delete package-lock.json and do
`npm install` to rebuild it. This commit has the result of it.

Also this commit changes `lockfileVersion` in package-lock.json to 3,
which is backwards compatible to npm v7 (bundled with node starting from
version 15). We're using node16 in CI for package tests, so it's fine.

[1] https://github.com/ably/ably-js/actions/runs/7985041805/job/21802827410?pr=1625
[2] npm/cli#4828
@VeskeR VeskeR force-pushed the 1622/fix-ably-react-import branch from 86ee859 to 0bffeb7 Compare February 21, 2024 11:32
@github-actions github-actions bot temporarily deployed to staging/pull/1625/features February 21, 2024 11:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/bundle-report February 21, 2024 11:33 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1625/typedoc February 21, 2024 11:33 Inactive
Base automatically changed from 1619/fix-ably-import-in-react-hooks to integration/v2 February 21, 2024 20:14
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian left a comment

Choose a reason for hiding this comment

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

Please could you update test/package/browser/template/README.md to reflect your changes?

@VeskeR
Copy link
Contributor Author

VeskeR commented Feb 21, 2024

Please could you update test/package/browser/template/README.md to reflect your changes?

Sure! I totally missed it had a README

@VeskeR
Copy link
Contributor Author

VeskeR commented Feb 23, 2024

Updated README and npm scripts

@VeskeR VeskeR removed the request for review from ttypic February 26, 2024 16:59
@VeskeR VeskeR merged commit f33d4c0 into integration/v2 Feb 26, 2024
12 checks passed
@VeskeR VeskeR deleted the 1622/fix-ably-react-import branch February 26, 2024 17:00
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.

Can't import from 'ably/react' on v2 branch
2 participants