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: using all passed options #67

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

jaymorrow
Copy link
Contributor

Didn't see anything in the README about how to properly format PRs, please let me know if there's anything you would like me to do to validate this change.

Description

When testing the Github action my extension is not in a publishable state and the action kept failing (I wanted to just upload and not publish). After investigation I realized it was because the uploadOnly option was not being assigned to this.options in the constructor.

This change will still check for required fields but use all passed constructor options. If you'd like it to be more specific to only allow defined options I'm happy to make that change as well.

@jaymorrow jaymorrow changed the title using all passed options fix: using all passed options Jun 20, 2023
@louisgv
Copy link
Contributor

louisgv commented Jun 20, 2023

ooh great catch - now that we added more and more params, it seems that this assignment approach is much better than doing only required params (we're sharing the same interface for the other store submission API as well).

I will merge this and also prepare a quick fix for all of the store to use a similar strategy!

@louisgv
Copy link
Contributor

louisgv commented Jun 20, 2023

@jaymorrow actually - would you be down to fix it for all the store API? The idea is to validate the options for all of them. Also, I think we might want to use the spread operator instead:

this.options = {...options}

I'm in an era with very crappy wifi atm xD...

@jaymorrow
Copy link
Contributor Author

would you be down to fix it for all the store API? The idea is to validate the options for all of them. Also, I think we might want to use the spread operator instead:

Sure, I'll take a look at the others as well and include the same fix. And yeah, while it's hard to imagine someone changing a value on the options object dynamically spread does make it a bit safer, will do :)

@jaymorrow
Copy link
Contributor Author

Sure, I'll take a look at the others as well and include the same fix

Looked through the mozilla-addons-api constructor and the edge-addons-api constructor and this is the only one that doesn't have all of the current options covered.

Copy link
Contributor

@louisgv louisgv left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@louisgv louisgv merged commit b2deb2f into PlasmoHQ:main Jun 21, 2023
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

2 participants