Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: support frontend plugins via env.config.jsx #240
feat: support frontend plugins via env.config.jsx #240
Changes from all commits
69f3467
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iterate
can only be called on filters that return a list of things. I think that the annotation of PLUGIN_SLOTS is incorrect. It should be:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! This solved the linting cascade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hinakhadim, your previous comment got me on the right track, thanks - but I didn't want to change running code just for the sake of a type hint, so this is what I did. It seems there's significant precedent for
Any
in the codebase, so I expect it to be fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is caused by the lru_cache decorator, which is not correctly typed. You can bypass this issue without
Any
by writing:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Is this idiomatic? Shouldn't it be
if(!(slot_name in config.pluginSlots))
? Orif(config.pluginSlots[slot_name] === null)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only
if (slot_name in config.pluginSlots)
is fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On syntax: when testing for whether properties exist or not, Javascript is a minefield. You have to make sure you're not testing the value of the property if it exists. This is one of the reasons why the
in
operator came about.And truthiness is also a minefield in Javascript.
=== false
is the only way to be absolutely sure. It's why MDN uses it in their example.Either way, we do have to use the negative case, here. It avoids duplicating code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason for adding plugin_config object one by one?
Could we pass array for one plugin_slot (keeping in mind multiple plugins can have same slots)?
Like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I just realized that objects are already passing in array. I need to pass without array brackets:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are different ways you can load the configuration in. @regisb suggested we document it one-by-one to avoid issues with trailing commas. Though in this latest iteration of
env.config.jsx
it doesn't really matter if there are trailing commas or not, I guess it keeps it simple.Otherwise, yes you can totally use a single
add_item()
with a string that has multiple ops for one slot.