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

Basic support for style and script preprocessing. #959

Closed
wants to merge 1 commit into from

Conversation

esarbanis
Copy link
Contributor

Suggestion for #181 and #876

@esarbanis esarbanis changed the title Basic support for css preprocessing. No support for async preprocessing and multiple <style> tags Basic support for markup, style and script preprocessing. Nov 25, 2017
@esarbanis esarbanis force-pushed the css-preprocessing branch 2 times, most recently from 369ebab to 04384d8 Compare November 25, 2017 22:45
@codecov-io
Copy link

codecov-io commented Nov 25, 2017

Codecov Report

Merging #959 into master will decrease coverage by 0.03%.
The diff coverage is 83.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #959      +/-   ##
==========================================
- Coverage   91.76%   91.72%   -0.04%     
==========================================
  Files         109      109              
  Lines        4054     4083      +29     
  Branches     1301     1303       +2     
==========================================
+ Hits         3720     3745      +25     
- Misses        150      154       +4     
  Partials      184      184
Impacted Files Coverage Δ
src/index.ts 81.96% <83.87%> (+0.71%) ⬆️
src/generators/Generator.ts 94.21% <0%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72bd23a...953c296. Read the comment docs.

@esarbanis esarbanis force-pushed the css-preprocessing branch 7 times, most recently from d7a9f65 to 5882e55 Compare November 26, 2017 15:24
@esarbanis
Copy link
Contributor Author

From what I can understand travis-ci/pr is failing due to network issues and the code coverage change is due to skipping tests that cannot be run on node version earlier than 8.

@Rich-Harris any suggestions on those issues?

@Conduitry
Copy link
Member

I'm very leery about introducing all of these preprocessing libraries into Svelte itself. I don't think the core library should be at all responsible for knowing how to, say, translate coffeescript to js or sass to css. I'm not sure how this should look, but I feel like it should either be in separate libraries that function as plugins to Svelte somehow - or maybe the third-party preprocessing libraries could be made to work as optional peer dependencies somehow. I really don't like bundling all of them in to the compiler - and for things like node-sass, I'm not even sure how bundling would work, since it requires compiling and running native code.

@esarbanis
Copy link
Contributor Author

I totally agree with this approach, that's why svelte.preprocess, which is in core, is implementation agnostic and based on the recommendation of @Rich-Harris in #876.

All preprocessing libraries are introduced in dev scope and used in testing to showcase the versatility of the solution.

I had to use markup, style and script preprocessors to achieve good testing coverage. Maybe using 3 css preprocessors is an overkill and I could leave just one.

@esarbanis esarbanis changed the title Basic support for markup, style and script preprocessing. Basic support for style and script preprocessing. Nov 27, 2017
@esarbanis
Copy link
Contributor Author

Markup preprocessing was redundant as svelte components are expressed in markup anyway, so by piping a markup preprocessor with the compiler directly would have the same results, without the need for core to support such a thing.

So I just removed that part completely from the proposal.

@esarbanis esarbanis force-pushed the css-preprocessing branch 2 times, most recently from 17c904a to aba2657 Compare November 29, 2017 18:17
@Rich-Harris
Copy link
Member

Thanks! There's really no need to include any preprocessors in this repo in order to test the behaviour of preprocess, even as devDependencies — they'll just slow up CI etc. So I've removed them in a second PR based on this one --> #969. I'll close this in favour of that.

@Rich-Harris Rich-Harris closed this Dec 3, 2017
@esarbanis esarbanis deleted the css-preprocessing branch December 3, 2017 15:38
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.

4 participants