Multi plugin islands configs #2275
Conversation
|
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:
|
|
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 |
tests/plugin_test.ts
Outdated
| @@ -182,6 +182,35 @@ Deno.test({ | |||
| sanitizeResources: false, | |||
| }); | |||
|
|
|||
| Deno.test({ | |||
| name: "plugin supports multiple islands", | |||
There was a problem hiding this comment.
This is a bit pedantic, but plugins already support multiple islands, because PluginIslands.paths is a string[]. What if you rename the test to "plugins support multiple island configs" or something like that. I think it would then be more clear what the test is asserting.
| { | ||
| baseLocation: import.meta.url, | ||
| paths: ["./sample_islands/IslandFromPlugin.tsx"], | ||
| }, |
There was a problem hiding this comment.
This is interesting because it essentially duplicates the island registered from route-plugin.ts. As I was stepping through the code, I saw what was about to happen and kept thinking "no problem, it'll get cleared out later". But it doesn't! But it also doesn't cause problems... 🤔
I guess it's not a terrible thing to have it like this, because sooner or later someone is bound to end up in a similar situation. I've made note of this in #1568 (comment) that we need to detect island conflicts like this. Although this is probably a special case in that the "conflict" isn't an issue, because everything is the same.
There was a problem hiding this comment.
Any changes to make for this?
There was a problem hiding this comment.
Nothing to do. I just wanted to highlight that this is happening and isn't causing issues, even though it's unexpected.
|
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/ |
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
baseLocationdoes not work.To prevent breaking changes, the plugin.islands value is updated to support a single PluginIslands config or an array of them.