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 Signature Error and Refactor saveConfiguration #20

Merged
merged 4 commits into from
Jun 4, 2023
Merged

Fix Signature Error and Refactor saveConfiguration #20

merged 4 commits into from
Jun 4, 2023

Conversation

Sevichecc
Copy link
Contributor

@Sevichecc Sevichecc commented Jun 3, 2023

There is a signature error in the getConfiguration method:

image
Uncaught (in promise) TypeError: Error in invocation of storage.get(optional [string|array|object] keys, function callback): No matching signature.
    at getConfiguration (configuration.js:8:32)
    at isConfigurationComplete (linkding.js:4:4)
    at background.js:33:10

And this PR aims to fix it and refactor the saveConfiguration method to be cleaner.

Copy link
Owner

@Fivefold Fivefold left a comment

Choose a reason for hiding this comment

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

Huh, interesting. I never got this error and can't reproduce it (on Windows 10). Maybe another OS-specific thing? Your changed code works just as well on Windows though (i.e. triggers no errors and can successfully fetch the configuration)

Pending the changes I'm happy to merge this.

src/configuration.js Outdated Show resolved Hide resolved
src/configuration.js Outdated Show resolved Hide resolved
src/configuration.js Outdated Show resolved Hide resolved
src/configuration.js Outdated Show resolved Hide resolved
@Sevichecc
Copy link
Contributor Author

The code has been refactored based on the review. Any suggestion?

@Sevichecc
Copy link
Contributor Author

Huh, interesting. I never got this error and can't reproduce it (on Windows 10). Maybe another OS-specific thing? Your changed code works just as well on Windows though (i.e. triggers no errors and can successfully fetch the configuration)

Pending the changes I'm happy to merge this.

The error is a bit weird, and I have spent some time figuring it out. It might be an OS-specific issue, or it might not be. Probably something is wrong with my local environment.

Having the config outside of the getConfiguration() function allows exporting down the line if needed.
@Fivefold Fivefold merged commit 9e0bf7b into Fivefold:master Jun 4, 2023
@Fivefold
Copy link
Owner

Fivefold commented Jun 4, 2023

The code has been refactored based on the review. Any suggestion?

Sorry, I wasn't clear enough with "outside the function" in #20 (comment)

I added the change I intended myself, no worries.

The error is a bit weird, and I have spent some time figuring it out. It might be an OS-specific issue, or it might not be. Probably something is wrong with my local environment.

Maybe, but if this fixes it, it doesn't matter in the end.


Thanks again for your time and work, I appreciate it!

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