florian-lefebvre / astro-integration-kit

A package that contains utilities to help you build Astro integrations.
https://astro-integration-kit.netlify.app
MIT License
56 stars 8 forks source link

feat: update `addVirtualImport` to `addVirtualImports` #69

Closed BryceRussell closed 7 months ago

BryceRussell commented 7 months ago

Closes #65

  addVirtualImports({
    updateConfig,
-   name: 'virtual:my-integration/config',
-   content: `export const config = ${JSON.stringify(config)}`,
+  name: 'my-integration',
+  imports: {
+    "virtual:my-integration/config": `export const config = ${JSON.stringify(config)}`
+  },
  })

I got to hop off so I am creating this PR for now, I still have to update docs and then it should all be good. Let me know what you think!

netlify[bot] commented 7 months ago

Deploy Preview for astro-integration-kit ready!

Name Link
Latest commit 414d6a5e288bea306c2f616f55a507484b556367
Latest deploy log https://app.netlify.com/sites/astro-integration-kit/deploys/65e201962f87730008c5ea22
Deploy Preview https://deploy-preview-69--astro-integration-kit.netlify.app/utilities/add-virtual-import
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

BryceRussell commented 7 months ago

Responding to this comment from the issue:

Fryuni: I think the Vite plugin could be the name of the integration, with maybe a count if it is called multiple times.

This would only be possible inside the plugin and not the standalone utility but I like this idea. Does the name of the Vite plugin get exposed to the user in any way like in an error? If so it may be hard to debug a name like vite-plugin-my-integration-2.

I have a working change for this I can push if this is something that should be included for the plugin

florian-lefebvre commented 7 months ago

The utility can still accept an explicit name prop, it's up to use to document it! As for the number, I don't know what to think about it. I agree it's not really great. Could it be possible instead to compute the plugin name based on the integration + the imports keys? This way we're pretty sure it's unique

BryceRussell commented 7 months ago

Ya no problem, I plan on doing the docs/examples for this later today after work

florian-lefebvre commented 7 months ago

Would you mind also resolving the conflicts? Shouldn't be too hard tbh

BryceRussell commented 7 months ago

Ya should be all fixed up with the new update. The only problem I am seeing is some Astro version mismatches on the playground test integrations

BryceRussell commented 7 months ago

Should I push the changes for inferring the Vite plugin name from the integration name with an incrementing number? I wrote the docs for the plugin without using the name property assuming we would want to support this somehow. We could infer the Vite plugin name using the keys of imports but it would have the same problem as the incrementing number and make it more complicated

BryceRussell commented 7 months ago

I went ahead pushed the changes for the incrementing plugin name and we can always discuss and change if there is better way