fresh
fresh copied to clipboard
Multi plugin islands configs
This PR is a simple change that would allow the plugin islands config to contain multiple configurations of islands. The purpose of this is to support configuring islands from shared libraries. We have an architecture for our atomic design kit that promotes composition of the final atomic library from base libraries. The issue is that the sub libraries that are composed together all have individual import.meta.url values that need to be used, so a single baseLocation
does not work.
To prevent breaking changes, the plugin.islands value is updated to support a single PluginIslands config or an array of them.
This sounds cool, although I haven't had time to look in detail. But after a quick glance at the changes, there are some basics that need taking care of:
- Don't update the current documentation -- that will cause confusion. Instead, branch
plugins.md
to/docs/canary/concepts/
and then updatetoc.ts
appropriately. You should be able to see the different versions when you dodeno task www
. This way people won't find out about the feature before it's released - What about some tests? Something in
/tests/fixture_plugin/
is probably a good starting point.
Will do, thanks for the input. I'll try to work this in today.
I got the docs updated (i think correctly). In terms of testing, i was looking at it more in terms of code coverage, and due to how the code is rewritten, the existing test covers the new code. Should i look at a test that actually utilizes an external library? or given that coverage is 100% with my changes, leave it as is?
Regardless of what the code coverage says, if you're adding a new type and not using it, then in my opinion it's not covered. At minimum from a "usage guide" (i.e. how does one use this) perspective I would expect to see a plugin that uses your nice new type in this change. This also helps from a type checking perspective, to make sure we catch any possible regressions with code that users have defined.
So something like this:
{name: "multipleIslandsLocationPlugin",
islands: [{baselocation: "firstPlace", paths: ["a", b"]}, {baselocation: "secondPlace", paths: ["c", d"]}]
}
But since I can't merge your change, my opinion is just an opinion. Feel free to ignore me and wait for Marvin 😄
Thank you for your thoughts, much appreciated, I'll work to get this test in place so we can look at merging this in.
Merged in latest
Yes, but what about the tests?
Shoot, had several contributions going in different repos and forgot to do the tests here, i'll work to get those in today.
Alright, i added a second island in a "sub" folder, and a new plugin that uses it with the original island. The test is plugin supports multiple islands
and seems to be passing. Not sure if this is exactly how the test should be configured, so open to input on where it belongs and how it should be written.
I can confirm that this is working in multiple deployed applications. One easy to use example is mydiviner.ai. Also using it with https://dashboard.openbiotech.co/