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.

Sign up for GitHub

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

Jump to bottom

feat(sveltekit): upgraded sveltekit support to svelltekit v1.0.0 (fix #392, #397) #402

Merged
merged 4 commits into from
Dec 19, 2022
Merged

feat(sveltekit): upgraded sveltekit support to svelltekit v1.0.0 (fix #392, #397) #402

merged 4 commits into from
Dec 19, 2022

Conversation

bhvngt
Copy link
Contributor

@bhvngt bhvngt commented Dec 15, 2022

Description

This PR makes histoire compatible to recent changes in the svelte v1.0.0 They are

  • Changes in the vite ignore plugins. see ref from @benmccann
  • Fixed promise resolution of plugin option that was compelling svelte kit users to await sveltekit plugin
  • migrated sveltekit example to v1.0.0

With above changes, histoire should be able to work with svelte kit v1.0.0 without any changes to their setup.

This PR is a part duplicate of #393, which was addressing the first of the above issues. Shout out to @vnphanquang for initiating this effort.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • If it's a new feature, provide a convincing reason to add it. Ideally, you should open a suggestion issue first and have it approved before working on it.
  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bhvngt
fix(plugin-sveltekit): upgraded sveltekit support to svelltekit v1.0.0.
6f93a28 
 - changed viteIgnorePlugins to vite-plugin-sveltekit-compile
 - fixed pluginOption awaiting logic
 - upgraded sveltekit example to `v1.0.0`

Closes: #392,#397
@codesandbox
Copy link

codesandbox bot commented Dec 15, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@stackblitz
Copy link

stackblitz bot commented Dec 15, 2022

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Dec 15, 2022 •
edited
Loading

✅ Deploy Preview for histoire-examples-svelte3 ready!

Name Link
🔨 Latest commit 526d726
🔍 Latest deploy log https://app.netlify.com/sites/histoire-examples-svelte3/deploys/63a068f2b25d2f00088e8fa5
😎 Deploy Preview https://deploy-preview-402--histoire-examples-svelte3.netlify.app
📱 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 settings.

@netlify
Copy link

netlify bot commented Dec 15, 2022 •
edited
Loading

✅ Deploy Preview for histoire-controls ready!

Name Link
🔨 Latest commit 526d726
🔍 Latest deploy log https://app.netlify.com/sites/histoire-controls/deploys/63a068f20cc5690009f1bd32
😎 Deploy Preview https://deploy-preview-402--histoire-controls.netlify.app
📱 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 settings.

@netlify
Copy link

netlify bot commented Dec 15, 2022 •
edited
Loading

✅ Deploy Preview for histoire-site ready!

Name Link
🔨 Latest commit 526d726
🔍 Latest deploy log https://app.netlify.com/sites/histoire-site/deploys/63a068f2d38e6400076cb61d
😎 Deploy Preview https://deploy-preview-402--histoire-site.netlify.app
📱 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 settings.

@netlify
Copy link

netlify bot commented Dec 15, 2022 •
edited
Loading

✅ Deploy Preview for histoire-examples-vue3 ready!

Name Link
🔨 Latest commit 526d726
🔍 Latest deploy log https://app.netlify.com/sites/histoire-examples-vue3/deploys/63a068f29bdc65000886dd6d
😎 Deploy Preview https://deploy-preview-402--histoire-examples-vue3.netlify.app
📱 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 settings.

@vnphanquang vnphanquang mentioned this pull request Dec 15, 2022
Closed
10 tasks
@vnphanquang
Copy link

Great work @bhvngt. I've closed my PR in favor of this. Thanks a lot.

benmccann
benmccann reviewed Dec 15, 2022
View reviewed changes
packages/histoire/src/node/vite.ts
@@ -78,10 +78,11 @@ export async function mergeHistoireViteConfig (viteConfig: InlineConfig, ctx: Co
let flatPlugins = []
if (viteConfig.plugins) {
for (const pluginOption of viteConfig.plugins) {
if (Array.isArray(pluginOption)) {
flatPlugins.push(...await Promise.all(pluginOption))
const resolvedPluginOption = await pluginOption
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this changed. Now if it's an array it's being awaited twice

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I guess awaiting twice is actually correct: https://github.com/vitejs/vite/blob/af86e5bcfe4b78f486f499cba09c3270fb151d54/packages/vite/src/node/config.ts#L111

Copy link
Member

Choose a reason for hiding this comment

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

A vite plugin can be of the form: [promise, promise, promise]

benmccann
benmccann approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

Thanks!!

@benmccann
Copy link
Contributor

@Akryum anything else we need to do to merge this PR?

@Akryum
Copy link
Member

Akryum commented Dec 19, 2022

@benmccann This PR was pending the merge of the vite 4 branch 😉

@Akryum Akryum changed the title fix(plugin-sveltekit): upgraded sveltekit support to svelltekit v1.0.0 (fix #392, #397) feat(sveltekit): upgraded sveltekit support to svelltekit v1.0.0 (fix #392, #397) Dec 19, 2022
@Akryum Akryum merged commit e7e5993 into histoire-dev:main Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Akryum Akryum Akryum left review comments

@benmccann benmccann benmccann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bhvngt @vnphanquang @benmccann @Akryum