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 usage of assert.invariant #298

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

eliandoran
Copy link
Contributor

I was using php-serialize at client level using Svelte and Vite, when it warned me at build time:

[plugin:vite:resolve] Module "assert" has been externalized for browser compatibility, imported by "/home/elian/Projects/toolkit/node_modules/php-serialize/lib/esm/unserialize.js". See http://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.

Apparently this is due to the fact that assert is a Node module, so it has to embed this module to work in the browser.

I've looked at the uses of invariant in the code base and they are only a few, so I thought about replacing them with simple guard statements instead.

@steelbrain
Copy link
Owner

Hello! Thanks for taking the time to contribute. Instead of inlining these checks, how do you feel about creating a helper function that broadly matches the signature of invariant and use that instead?

@eliandoran
Copy link
Contributor Author

@steelbrain , sure, no problem with that.

I saw there were only a few of them, but maybe it's more elegant to have a helper function.

@eliandoran eliandoran marked this pull request as draft June 30, 2024 16:47
@eliandoran eliandoran force-pushed the feature/remove_invariant branch from 51d8735 to 80fd44d Compare June 30, 2024 17:15
@eliandoran eliandoran marked this pull request as ready for review June 30, 2024 17:16
@eliandoran
Copy link
Contributor Author

eliandoran commented Jun 30, 2024

@steelbrain , done.

I've noticed that the tests pass even if I negate the if in invariant, so it might be worth checking if it's possible to improve the code coverage. Never mind, apparently I had to run npm run prepare before npm run test.

Copy link
Owner

@steelbrain steelbrain left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, minor nit and then we're good

src/helpers.ts Outdated Show resolved Hide resolved
@eliandoran eliandoran force-pushed the feature/remove_invariant branch from 80fd44d to d640767 Compare June 30, 2024 17:49
@eliandoran eliandoran marked this pull request as draft June 30, 2024 18:44
@eliandoran
Copy link
Contributor Author

eliandoran commented Jun 30, 2024

Apparently the yarn lint command fails for some reason.

EDIT: It was the linter, apparently I did not notice that the project does not use semicolons.

This reduces dependencies when building for the browser target, since
the assert module is a Node.js dependency.
@eliandoran eliandoran force-pushed the feature/remove_invariant branch from d640767 to 6981e32 Compare June 30, 2024 18:49
@eliandoran eliandoran marked this pull request as ready for review June 30, 2024 18:49
Copy link
Owner

@steelbrain steelbrain left a comment

Choose a reason for hiding this comment

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

Thanks again!

@steelbrain steelbrain merged commit fd2ac5b into steelbrain:main Jul 1, 2024
@eliandoran eliandoran deleted the feature/remove_invariant branch July 1, 2024 14:14
@steelbrain
Copy link
Owner

Published in https://github.com/steelbrain/php-serialize/releases/tag/v5.0.0

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.

2 participants