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

Remove the assert lib #117

Merged

Conversation

nandito
Copy link
Collaborator

@nandito nandito commented Nov 6, 2023

  • Bump jslib-media, the new version:
    • Adds media device and constraints helpers
    • Replaces the 3rd party assert lib
  • Import assert from jslib-media and remove the assert lib from browser-sdk's dependencies.
  • Remove some of the redundant type checks.

The assert lib replacement should fix the "Could not find module in path: 'assert/assert.js'" error in CodeSandbox.

Test plan

  1. Install the new browser-sdk with different bundlers/frameworks (cra, vite, etc)
  2. Make sure you don't get the "Could not find module" error for assert.

Related PR

whereby/jslib-media#32

@nandito nandito requested a review from a team November 6, 2023 10:50
@nandito nandito self-assigned this Nov 6, 2023
Copy link
Member

@thyal thyal left a comment

Choose a reason for hiding this comment

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

Nice. But we also use assert directly in this repo - maybe we can replace the usage in this PR as well?

@nandito
Copy link
Collaborator Author

nandito commented Nov 6, 2023

Nice. But we also use assert directly in this repo - maybe we can replace the usage in this PR as well?

Yeah I just realized this now. Actually we use it way more here than in jslib-media. 🙈
I'm going to remove it from here too. 🚧 👷‍♂️

@nandito nandito marked this pull request as draft November 6, 2023 10:55
@kevinwhereby
Copy link
Contributor

kevinwhereby commented Nov 6, 2023

Actually we use it way more here than in jslib-media. 🙈

I reckon a lot of it is redundant with the move to typescript in this repo, so my vote would be we remove calls to it where possible as well.

I.e. all the assertOk(typeof thing === "thingtype")

@nandito
Copy link
Collaborator Author

nandito commented Nov 6, 2023

Actually we use it way more here than in jslib-media. 🙈

I reckon a lot of it is redundant with the move to typescript in this repo, so my vote would be we remove calls to it where possible as well.

I.e. all the assertOk(typeof thing === "thingtype")

I agree! I have removed some now, but there are still quite a few remaining. I'm not sure if I want to increase the scope, as the original task was to remove the 3rd party assert and I have already done that. Therefore, bundlers, tools, and frameworks should no longer complain about the missing module.

@nandito nandito marked this pull request as ready for review November 6, 2023 13:16
@nandito nandito changed the title Bump jslib-media version (minor) Remove assert lib Nov 6, 2023
@nandito nandito changed the title Remove assert lib Remove assert lib Nov 6, 2023
@nandito nandito changed the title Remove assert lib Remove the assert lib Nov 6, 2023
This new version:
* Adds media device and constraints helpers
* Replaces the 3rd party `assert` lib

The `assert` lib replacement should fix the "Could not find module in
path: 'assert/assert.js'" error in CodeSandbox.
TypeScript already checks the types, so no need to explicitly re-check
them with `assert.ok` or `assertString`.
@nandito nandito force-pushed the nandor/pan-502-remove-the-assert-lib-from-jslib-media branch from 9b1a273 to 7f4b4eb Compare November 6, 2023 15:53
Copy link
Member

@thyal thyal left a comment

Choose a reason for hiding this comment

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

🍰 Works nicely.

@nandito nandito merged commit 9e349e0 into development Nov 7, 2023
2 checks passed
@nandito nandito deleted the nandor/pan-502-remove-the-assert-lib-from-jslib-media branch November 7, 2023 09:08
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.

3 participants