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.

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

Builder-Vite: use user's build.target #23123

Conversation

Hoishin
Copy link
Contributor

@Hoishin Hoishin commented Jun 18, 2023

Closes #22223

What I did

If user has build.target in Vite config, use it for Storybook's Vite config.

How to test

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@valentinpalkovic valentinpalkovic self-assigned this Jun 19, 2023
@valentinpalkovic
Copy link
Contributor

Hi @Hoishin,

Thank you very much for this PR.

I don't feel very confident about merging this PR. This change could potentially cause the application to stop working. Therefore, since this is a potential breaking change, I would like to merge this PR for SB 8.0 after internal consultation.

Until then, the already mentioned workaround can be used:

async viteFinal(config) {
    return mergeConfig(config, {
      build: {
        target: 'esnext'
      },
    });
  },

I hope you understand our worries.

@IanVS
Copy link
Member

IanVS commented Sep 29, 2023

This change could potentially cause the application to stop working.

@valentinpalkovic do you mean to say that storybook cannot be built with that target? Or when you say "cause the application to stop working", do you somehow mean the user's application itself?

I do agree that this is likely best left to 8.0, just curious if there are specific concerns about storybook being incompatible with different targets other than the default.

@valentinpalkovic
Copy link
Contributor

@IanVS As far as I remember, we discussed this internally. We concluded that merging this PR could potentially break Storybook since the target might differ from the default one, and unexpected behavior might pop up. This is a precaution rather than basing the decision on some incidents in the past.

@valentinpalkovic valentinpalkovic changed the base branch from next to release-8-0 October 2, 2023 11:13
@IanVS
Copy link
Member

IanVS commented Oct 2, 2023

since the target might differ from the default one

This makes sense, but on the other hand if Storybook is using a target that is different from the one specified in the user's vite config, I could also see that as being a bug, since their SB won't match their application. Personally I think it wouldn't be bad to release it before 8.0, but don't feel strongly one way or the other, especially since there's a pretty straightforward workaround.

@valentinpalkovic valentinpalkovic changed the title fix(builder-vite): use user's build.target Builder-Vite: use user's build.target Oct 5, 2023
@valentinpalkovic valentinpalkovic merged commit b8f97f0 into storybookjs:release-8-0 Oct 5, 2023
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Overwriting "build" object in vite config in build mode prevents use of top-level await
3 participants