Conversation
…ctor tests to use React Testing Library instead of Enzyme, improving test reliability and maintainability. Update coverage summary to reflect changes in test structure.
|
❗ Build for commit 830ecd1 did not complete. |
|
❗ Build for commit 595c868 did not complete. |
|
✅ Build for commit 595c868 passed. |
|
✅ Build for commit aa9edf0 passed. |
|
✅ Build for commit 2b14e3c passed. |
|
✅ Build for commit b70514f passed. |
There was a problem hiding this comment.
We should .gitignore this file and remove it from git instead. I think you already advise using npm run demo to generate/run it.
There was a problem hiding this comment.
likely need to remove and .gitignore this file as well
| <header> | ||
| <div class="badge-row"> | ||
| <span class="badge react">React 19</span> | ||
| <span class="badge version">jwplayer-react v1.1.3</span> |
There was a problem hiding this comment.
Should remove the version so that it's not out of date.
There was a problem hiding this comment.
Install & Test
| - name: Install & Test |
| "main": "./lib/jwplayer-react.js", | ||
| "scripts": { | ||
| "build": "webpack && npm run badges:build:passing || npm run badges:build:failing", | ||
| "ci": "npm ci; npm run build; npm run test; npm run lint; npm run badges:license", |
There was a problem hiding this comment.
This can never fail. Should be
| "ci": "npm ci; npm run build; npm run test; npm run lint; npm run badges:license", | |
| "ci": "npm run build && npm run test && npm run lint && npm run badges:license", |
| "dependencies": { | ||
| "@jwplayer/jwplayer-react": "^1.1.1" | ||
| } | ||
| "dependencies": {} |
There was a problem hiding this comment.
nit: delete
| "dependencies": {} |
| "typescript": "5.0.4", | ||
| "webpack": "5.69.1", | ||
| "webpack-cli": "4.9.2" | ||
| "jwplayer-react-from-npm": "npm:@jwplayer/jwplayer-react@^2.0.0", |
There was a problem hiding this comment.
Do we need this? Kinda weird to install your own package.
jwbrandon
left a comment
jwbrandon
left a comment
There was a problem hiding this comment.
I think we might want to add a check in ci that runs a test to make sure that we support 17/18 still unless we are dropping those in favor of only supporting 19.
- fail ci on build/test/lint errors and run them in parallel via npm-run-all - untrack demo/bundle.js and coverage output; gitignore both - drop the jwplayer-react-from-npm self-dependency and empty dependencies map - remove the hardcoded version from the demo page badge - bump @typescript-eslint to v8 for typescript 5.7 support - add a react 17/18 compat smoke test and run it as a ci step - bail out of componentDidMount when the component unmounts (or StrictMode remounts) while the player library is still loading
|
❗ Build for commit 9dc07ed did not complete. |
The license badge and LICENSE.md have said ISC since 2022; the MIT value in package.json was boilerplate and is what npm advertises. Match the actual license grant.
…t 19 compat - bump version to 2.0.1 (2.0.0 is already published without these fixes) - ship TypeScript types: rewrite src/jwplayer-react.d.ts as a valid standalone declaration, copy it into lib/ at build time, add the types field - restrict the badge auto-commit to push events and badges/*.svg, drop --force and the ignored token input - run the compat smoke test against react 19 as well, and gate it on didMountCallback instead of a fixed timeout - pin the published build to browserslist defaults instead of the builder's node version (test env keeps node targets for jest) - re-scope the StrictMode regression test to the async-load path the guard actually covers, and drop order-dependent test globals - add @testing-library/dom as an explicit devDependency (RTL 16 peer) - use the TS-aware no-unused-vars rule for src/*.ts declaration files - exclude demo.html and webpack.demo.js from the npm tarball
A strict tsc pass over a consumer snippet (test/types/consumer.tsx) validates src/jwplayer-react.d.ts, which is copied verbatim into lib/ at build time. Runs as test:types inside npm run ci.
Cache the in-flight library load per URL so StrictMode remounts or multiple players sharing a library reuse one script tag instead of injecting duplicates; the entry is dropped once its script leaves the DOM so a failed or removed load can be retried. Wrap the mount-time load in try/catch so a failed library fetch logs instead of surfacing as an unhandled rejection in the consumer app. Drop the dead props.ref fallback (refs never arrive via props on class components).
Type the component's construct signature to return a JWPlayerInstance exposing player and id, so ref.current.player typechecks per the documented advanced-usage pattern. Cover it in the types consumer test.
Stop committing the build output: gitignore lib and rely on prepublishOnly to rebuild it into the publish tarball. Exclude badges from the package. Constrain the react peer range to the tested 17/18/19 majors and declare engines node >=18 to match the README.
Use prepare (not prepublishOnly) so lib is built for both npm publish and git installs (npm i jwplayer/jwplayer-react#branch), whose main would otherwise point at a missing file. Split build:dist (webpack + types) so the prepare hook produces the artifact without generating badges. Skip the prepare rebuild during compat installs via --ignore-scripts; react is external, so the bundle is identical.
Detach the script element when a library load fails so a retry appends a fresh tag instead of orphaning the failed one in the DOM.
Add tests for shared-library script dedupe, failed-load logging with dead-script cleanup, and fresh-script retry after a failure.
Only evict the cache entry if it still points at this script, so a stale load failing late cannot drop a newer retry's live entry.
Restore mocks in afterEach so a failed assertion cannot leak a console spy into later tests, and cover the retry-race eviction guard.
4 participants
Upgrade to react v19
Addresses Issue(s):
#32