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

chore: Remove gl devDependency #829

Merged
merged 1 commit into from
Oct 3, 2024
Merged

chore: Remove gl devDependency #829

merged 1 commit into from
Oct 3, 2024

Conversation

manzt
Copy link
Member

@manzt manzt commented Oct 2, 2024

I'd assumed that this dependency was somehow used in our test suite to mock WebGL but it doesn't appear like removing it causes any problems to CI or running tests locally. I say we remove it.

See:

cc: @xinaesthete

@xinaesthete
Copy link

I think the tests effectively inherit from how luma.gl does its tests? I haven't looked into that in depth but seem to remember seeing puppeteer mentioned in some logs as I glanced over - so it may be that earlier versions of your tests used gl more directly but now hopefully WebGPU when it's used would also be covered without any particular changes to viv.

Anything that did use gl would be more surface area to consider as that kind of refactoring happened, so I think you want to be explicitly aware of whether it's actually doing anything useful and removing seems good.

@ilan-gold
Copy link
Collaborator

I think the tests effectively inherit from how luma.gl does its tests? I haven't looked into that in depth but seem to remember seeing puppeteer mentioned in some logs as I glanced over - so it may be that earlier versions of your tests used gl more directly but now hopefully WebGPU when it's used would also be covered without any particular changes to viv.

This would have been my assessment as well. If nothing breaks, I'd say it's fine to remove.

@manzt manzt merged commit d700f7d into main Oct 3, 2024
5 checks passed
@manzt manzt deleted the manzt/remove-gl branch October 3, 2024 12:16
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