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

Read svelte.config.js #428

Merged
merged 4 commits into from
Jul 8, 2022
Merged

Read svelte.config.js #428

merged 4 commits into from
Jul 8, 2022

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jul 6, 2022

Partially reverts #382

The reason given there was that:

which may not be what storybook users want / expect, since their production config may be different from the storybook config (same reason we don't automatically merge vite.config.js)

However, I think that users would expect their svelte.config.js to be taken into account. E.g. right now the user's preprocessing configuration is ignored. The user can still pass custom svelteOptions in .storybook/main.cjs if they need to override their normal options for some reason (although I can't think of a reason off the top of my head why this would be necessary)

You can see a number of people complaining preprocessing doesn't work here: storybookjs/addon-svelte-csf#4

@IanVS
Copy link
Member

IanVS commented Jul 6, 2022

I think this makes sense. I see it as a breaking change, but we will need a new breaking release soon anyway for vite 3 support, so it's good timing. @joshwooding do you have any concerns with automatically loading the config from svelte.config.js? I think it makes more sense especially now that the sveltekit config will be moving to vite.config.js, since as I understand it, sveltekit itself is mostly about routing and ssr and other "metaframework" concerns that aren't really important configs when showing components in storybook (though I could well be wrong about that).

@joshwooding
Copy link
Member

No concerns, it makes sense to me 👍🏽

@IanVS
Copy link
Member

IanVS commented Jul 6, 2022

Actually, without a svelteOptions in the storybook config, I think that this might break: https://github.com/storybookjs/builder-vite/blob/main/packages/builder-vite/svelte/csf-plugin.ts#L17.

@benmccann
Copy link
Contributor Author

Can you clarity why that might break? It looks like it checks if svelteOptions exists before using it

@IanVS
Copy link
Member

IanVS commented Jul 7, 2022

My concern is that by automatically using the config in svelte.config.js for the svelte plugin, users will not know that they need to set the preprocessor in svelteOptions, which is relied on by our csf-plugin that I linked to. Essentially, we'd need to also check for a svelte.config.js file and use its preprocessor in the csf-plugin. Maybe we can use the logic in the svelte plugin as a starting point. It would indeed be nice to be able to just use it, rather than forcing users to duplicate or import it manually.

@benmccann
Copy link
Contributor Author

Ah, right. Okay. I've updated that as well

@IanVS
Copy link
Member

IanVS commented Jul 7, 2022

Thanks, although the logic in the svelte plugin looks a fair deal more complex: https://github.com/sveltejs/vite-plugin-svelte/blob/main/packages/vite-plugin-svelte/src/utils/load-svelte-config.ts. It's too bad that loadSvelteConfig is not in the package.json exports, so I guess we can't import and use it directly. :(

@benmccann
Copy link
Contributor Author

Yeah, the logic there is more complex, but I'm not sure it really needs to be. All Svelte projects have had "type": "module" in the package.json since before svelte.config.js existed, so there's really no reason anyone should use svelte.config.cjs or svelte.config.mjs. While I suppose supporting them is fine, I actually think throwing an error might be the better behavior as it'd be extremely discouraged to use either of those file extensions with the Svelte config.

@IanVS
Copy link
Member

IanVS commented Jul 7, 2022

OK, I'm on board with this. Would you mind updating the svelte example to remove this line?

preprocess: preprocess(),

And I guess convert the example to "type": "module"? And since maybe this is more common that we might think, we should throw an error message if we can't find a svelte.config.js for svelte projects? Is every svelte project required to use such a config file?

@benmccann
Copy link
Contributor Author

It turns out that loadSvelteConfig is exported, so I've updated the PR to use it. I've updated the example to remove the preprocess line as well

@IanVS
Copy link
Member

IanVS commented Jul 7, 2022

How is that working? Is exports not being respected, I guess? https://github.com/sveltejs/vite-plugin-svelte/blob/7758a561b670541cc3cf24bc01adefb11eeff42c/packages/vite-plugin-svelte/package.json#L17-L24

Probably because we're using typescript to compile to commonjs...

@benmccann
Copy link
Contributor Author

It exports ., which is what we're importing (i.e. @sveltejs/vite-plugin-svelte)

@IanVS
Copy link
Member

IanVS commented Jul 7, 2022

Ohhh, right. Thanks.

@IanVS IanVS merged commit c055857 into storybookjs:main Jul 8, 2022
@benmccann benmccann deleted the configFile branch July 8, 2022 20:14
IanVS added a commit that referenced this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants