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

fix: initialise default load options in Magika class (npm/js) #52

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

TheMythologist
Copy link
Contributor

@TheMythologist TheMythologist commented Feb 16, 2024

This fixes the following error when using magika.load():

TypeError: Cannot destructure property 'modelURL' of 'undefined' as it is undefined.

The current workaround is to use magika.load({}), which does not seem intuitive, and is also not what's described in the documentation.


As a side note, I was thinking of creating a constructor method in the Magika class that will perform the Magika.load operation by default, passing in the load options into the constructor. What do you think?

@reyammer reyammer changed the title fix: initialise default load options in Magika class fix: initialise default load options in Magika class (npm/js) Feb 19, 2024
@reyammer
Copy link
Collaborator

Thanks for the PR! @invernizzi: thoughts?

@invernizzi
Copy link
Member

Absolutely, thanks!

@invernizzi invernizzi merged commit b864ef6 into google:main Feb 19, 2024
6 checks passed
@reyammer
Copy link
Collaborator

Reopening as there was a question on creating a constructor: "As a side note, I was thinking of creating a constructor method in the Magika class that will perform the Magika.load operation by default, passing in the load options into the constructor. What do you think?"

This feels similar to what we do in the python package, which seems reasonable.

@TheMythologist: do you confirm it's basically the same?

@invernizzi: Thoughts?

If we need to change the API, better to do it now / early on...

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.

None yet

3 participants