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

[docs] add docs for vite-plugin-svelte #2665

Merged
merged 19 commits into from
Nov 2, 2021
Merged

[docs] add docs for vite-plugin-svelte #2665

merged 19 commits into from
Nov 2, 2021

Conversation

Mlocik97
Copy link
Contributor

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

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Oct 22, 2021

⚠️ No Changeset found

Latest commit: a2ae6fc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dummdidumm
Copy link
Member

It's passed to vite-plugin-svelte , not rollup. Does it silence all warnings you filter out through that in both dev mode and build?

@benmccann benmccann requested review from bluwy, dominikg and dummdidumm and removed request for dummdidumm October 22, 2021 18:54
@benmccann benmccann changed the title Update 14-configuration.md [docs] add docs for onwarn Oct 22, 2021
@Conduitry
Copy link
Member

Above this list, it says that the values below are the defaults. Is this noop function the default value for onwarn? Where's that default coming from? I don't see onwarn mentioned anywhere else in this repo besides in one test.

@dominikg
Copy link
Member

Imho kit docs should not include/duplicate documentation for specific vite-plugin-svelte options.

The onwarn option is going to be moved to vitePlugin.onwarn once sveltejs/rfcs#59 lands

Maybe a generic note+link to plugin docs helps? That would avoid having to maintain docs in 2 locations

@dummdidumm
Copy link
Member

The onwarn option is going to be moved to vitePlugin.onwarn once sveltejs/rfcs#59 lands

I think onwarn as top level would make sense so language tools can listen to it, too. The rollup/webpack plugins could, too.

@dominikg
Copy link
Member

when and if onwarn becomes a standardized top level config key it should be documented properly in sveltejs/config and linked here.

Currently it is an implementation detail of vite-plugin-svelte (and rollup-plugin-svelte). I don't think we should add the actual option here, unless we also want to start adding all other options too, and some more vite ones while we're at it?

Why exactly is this needed? Did we recieve tons of questions about warnings and vite-plugin-svelte docs are not clear enough?

@Mlocik97
Copy link
Contributor Author

Mlocik97 commented Oct 23, 2021

Why exactly is this needed? Did we recieve tons of questions about warnings and vite-plugin-svelte docs are not clear enough?

I saw a quite lot of question about silencing warnings in Discord.

@dominikg
Copy link
Member

Why exactly is this needed? Did we recieve tons of questions about warnings and vite-plugin-svelte docs are not clear enough?

I saw a quite lot of question about silencing warnings in Discord.

Please give examples where ignoring the warning is the correct solution. Most of the time warnings do identify actual problems that should be fixed in the code that caused them instead of just ignoring. If there are common "false positives" we should try to avoid issuing that warning altogether instead of leaving it for the user to clean up.

@Mlocik97
Copy link
Contributor Author

Mlocik97 commented Oct 23, 2021

Please give examples where ignoring the warning is the correct solution.

I never said it's correct... I said it's what is often asked for in Discord.

I myself never used onwarn setting...

@bluwy
Copy link
Member

bluwy commented Oct 23, 2021

Yeah, I agree with dominik that adding just onwarn may be too specific to vite-plugin-svelte (at the current state). However it's still ideal to show how to configure onwarn if it's often a common question asked. I think a small section explaining that the config applies to vite-plugin-svelte too is a nice middle ground, per my comment.

FWIW I had usecases for onwarn too, e.g. silencing them on builds (easier to debug), and silencing warnings from third-party packages (checking warning file path contains node_modules).

@dominikg
Copy link
Member

dominikg commented Oct 26, 2021

I'm still hesitant to mentioning onwarn, the discussion above indicates it could be abused by novice users to ignore warnings to their own disadvantage.
A generic "there are additional options for vite-plugin-svelte, look here" note is both less effort in the long run and more helpful as the current state of this PR only mentions onwarn.

Copy link
Member

@dominikg dominikg left a comment

Choose a reason for hiding this comment

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

Please update to a generic message as discussed in the Conversation above

@Mlocik97 Mlocik97 changed the title [docs] add docs for onwarn [docs] add docs for vite-plugin-svelte Oct 27, 2021
Mlocik97 and others added 4 commits October 29, 2021 18:44
@benmccann benmccann added the documentation Improvements or additions to documentation label Oct 31, 2021
documentation/docs/14-configuration.md Outdated Show resolved Hide resolved
documentation/docs/14-configuration.md Show resolved Hide resolved
documentation/docs/14-configuration.md Outdated Show resolved Hide resolved
Copy link
Member

@ignatiusmb ignatiusmb left a comment

Choose a reason for hiding this comment

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

This PR is getting too involved for such a straightforward change, we don't want to overwhelm newcomers by having too much information in the face when reading through the docs.

A comment on the config example explaining that options from the plugin can also be passed should be enough, maybe update docs on plugin repo to have an example usage with SK if needed, I left a suggestion to that comment, I'll leave the plugin docs to @dominikg and @bluwy

documentation/docs/14-configuration.md Outdated Show resolved Hide resolved
Mlocik97 and others added 2 commits November 2, 2021 12:22
Co-authored-by: Ignatius Bagus <ignatius.mbs@gmail.com>
documentation/docs/14-configuration.md Outdated Show resolved Hide resolved
@benmccann benmccann merged commit 6cff204 into sveltejs:master Nov 2, 2021
@Mlocik97 Mlocik97 deleted the patch-1 branch November 2, 2021 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants