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

[plugin-svelte] reveal config option #1292

Merged

Conversation

bearcanrun
Copy link
Contributor

Changes

In plugin-svelte, reveals config option allowing dev to config where svelte.config.js is located. My use case is for both monorepo and testing. Started/mentioned in #1241. Happy to make any changes!

Testing

Existing plugin tests (mocked & unmocked) updated.

Docs

Added config option and expected value to Plugin Options

@bearcanrun bearcanrun requested a review from a team as a code owner October 12, 2020 20:40
@vercel
Copy link

vercel bot commented Oct 12, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/lm3ej09n7
✅ Preview: https://snowpack-git-plugin-svelte-public-config.pikapkg.vercel.app

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

LGTM overall, just a few comments.

Also to bike shed even more, I'm wondering if config is confusing as a file path string since the entire object is also meant to be your "svelte config". Maybe instead configFile? configFilePath? I'm leaning towards configFilePath but open to alternatives if you have strong opinion here.

const userSvelteConfigLoc =
sveltePluginOptions.__config || path.join(process.cwd(), 'svelte.config.js');

const userSvelteConfigLoc = path.resolve(process.cwd(), `${config}/svelte.config.js`);
Copy link
Owner

Choose a reason for hiding this comment

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

configuring the directory but not the file is always confusing to me as a user, so lets go all the way and configure the file:

// config is always absolute, or relative to the cwd
let {config = 'svelte.config.js', ...svelteOptions} = sveltePluginOptions || {};
const userSvelteConfigLoc = config || path.resolve(process.cwd(), `svelte.config.js`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with verbose clarity. I'll update to configFilePath and update that path + filename needs to be configured.

Copy link
Owner

Choose a reason for hiding this comment

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

Great! much appreciated

sveltePluginOptions.__config || path.join(process.cwd(), 'svelte.config.js');

const userSvelteConfigLoc = path.resolve(process.cwd(), `${config}/svelte.config.js`);

if (fs.existsSync(userSvelteConfigLoc)) {
Copy link
Owner

Choose a reason for hiding this comment

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

needed: if I tell you to look for a specific config file, and it doesn't exist, I would want to see an error thrown (or at least logged to console).

@FredKSchott
Copy link
Owner

LGTM! Thanks for the contrib!

@FredKSchott FredKSchott merged commit 5b375b3 into FredKSchott:master Oct 13, 2020
@bearcanrun bearcanrun deleted the plugin-svelte-public-config branch October 13, 2020 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants