Skip to content

ignore .css files when building route manifest - #4185

Closed
Rich-Harris wants to merge 1 commit intomasterfrom
gh-3997
Closed

ignore .css files when building route manifest - #4185
Rich-Harris wants to merge 1 commit intomasterfrom
gh-3997

Conversation

@Rich-Harris
Copy link
Copy Markdown
Member

fixes #3997. I think we can punt on the question of whether we want to treat all .js/.ts files similarly — I think the .css case is fairly unambiguous. If we decide to make it more expansive in future, .css files would be included in that, so this is forward-compatible

then again this could be a can of worms if people start asking about .scss and what-have-you...

Please don't delete this checklist! 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
Copy Markdown

changeset-bot bot commented Mar 2, 2022

🦋 Changeset detected

Latest commit: ecfd337

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

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

@Conduitry
Copy link
Copy Markdown
Member

I'm not sure where the line is between stuff that should happen in the default https://kit.svelte.dev/docs/configuration#routes function vs stuff that should be stripped out in a separate step.

@benmccann
Copy link
Copy Markdown
Member

benmccann commented Mar 3, 2022

What happens to file types besides .js/.ts and .css/.scss/etc.? Do we want to generate routes for .json files? I don't know what we do today, but I would expect that we don't and that they should just go into static/. It seems potentially safer to me to whitelist .js / .ts than to blacklist .css?

@Rich-Harris
Copy link
Copy Markdown
Member Author

If we include rather than exclude, then we need a way to make it extensible to handle edge cases like .res (ReScript) files and whatever other wacky shit people want to use

Do we want to generate routes for .json files?

no, they will always belong in static

@benmccann
Copy link
Copy Markdown
Member

Should I ask if we also need to exclude image files? I have a feeling we'll need a config option either way whether it's to add stuff like .res or .scss. It seems less common to use other JS formats like .res than the combined use of all other extensions like .scss, so it might be the option that works out-of-the-box more of the time.

@yanneves
Copy link
Copy Markdown

yanneves commented Mar 3, 2022

Just my two cents as another possible avenue, we could tidy up the error to say "hey, that .css file doesn't belong under routes/, consider static/ or somewhere outside routes/ if you want to import it"

The route manifest and this directory are doing a lot of heavy lifting, and it wasn't obvious from the outset that I was interfering with it - in retrospect, it makes sense why this broke, and I think noping this use case doesn't detract from the developer experience

I'd like to think that could also reduce collateral continuing to implement advanced features (looking at you, shadow endpoints) with this directory

@Rich-Harris
Copy link
Copy Markdown
Member Author

Opened #4197 as a possible alternative

@Rich-Harris
Copy link
Copy Markdown
Member Author

Closing this in favour of #4197

@Rich-Harris Rich-Harris closed this Mar 3, 2022
@Conduitry Conduitry deleted the gh-3997 branch March 5, 2022 21:47
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.

CSS file present in src/routes/ breaks production build

4 participants