Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Handle dynamic css imports that are also statically imported #1494

Merged
merged 1 commit into from
Sep 9, 2020

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Sep 8, 2020

Fixes #1493. See the issue for a description of the change

@benmccann benmccann force-pushed the repl-css branch 2 times, most recently from c98681b to 93ef7c1 Compare September 8, 2020 03:12
@benmccann benmccann marked this pull request as draft September 8, 2020 03:18
@benmccann benmccann marked this pull request as ready for review September 8, 2020 03:20
@benmccann benmccann force-pushed the repl-css branch 2 times, most recently from e6a1e46 to 4738857 Compare September 8, 2020 03:34
@benmccann benmccann changed the title Fix CSS for REPL Handle dynamic css imports that are also statically imported Sep 8, 2020
@Conduitry
Copy link
Member

Can we add a test for this as well? These were obviously weird edge cases that were never considered, and it would be nice to have some ability to guard against them going forward.

@benmccann
Copy link
Member Author

Ok, I updated the tests so that they fail on master and pass with this PR. I didn't need to write a new test but just added a new route that would cause existing tests to fail

@Conduitry
Copy link
Member

Oh nice, I see that this also makes rollup-dependency-tree a dev/bundled dependency.

@benmccann
Copy link
Member Author

My reason for moving it was that it's only used in building the application by Rollup / the Sapper build process and so I was thinking it was a dev dependency. E.g. if someone runs npm ci --prod then I figured this would stop it from being a dependency that's shipped to production. It works just fine either place I have it and I don't have much of an opinion on where to put it. Were you thinking I should move it back?

@Conduitry
Copy link
Member

No no, it's nice having it be bundled in instead. My understanding from the conversation here was that you were running into some issues when doing that, but it didn't seem very worthwhile to try to get to the bottom of it, because Sapper itself is only a dev dependency of apps that are written in it, and so built apps didn't have any additional dependencies.

I'm a bit confused by parts of your last comment. I would expect that the only difference for someone installing sapper into their app would be whether they get a bundled copy of rollup-dependency-tree or their package manager has to separately request and install it.

@benmccann
Copy link
Member Author

I think we may have been talking about different things when you said "external" in #1452 (comment). I originally thought you meant was there a reason to make it a library and get it from npm instead of just putting the source code in Sapper. But now I understand you mean devDependencies vs not, so I was answering a bit of a different question on the other thread.

Feel free to ignore my last comment too 😄 I get a bit confused sometimes about Sapper being both a builder and a runtime in the same project

Anyway, I don't think there's much difference whether this is in devDependencies or dependencies, so I'm happy to have it in either place

@Conduitry Conduitry merged commit c951d33 into sveltejs:master Sep 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Svelte REPL broken on svelte.dev
2 participants