css preprocessor plugins by jkuri · Pull Request #388 · angular/angular-cli

jkuri · 2016-04-02T04:22:12Z

No description provided.

gkalpak · 2016-04-02T17:42:50Z

+
+ options = options || {};
+ Plugin.call(this, inputNodes, {
+ cacheInclude: [/(.*?).scss$/, /(.*?).sass$/]


Shouldn't it be \.scss and \.sass ?

And the ? isn't necessary afaict.

jkuri · 2016-04-03T00:04:01Z

Updated, thanks for checking @gkalpak.

gkalpak · 2016-04-03T10:10:35Z

+ it('Installs sass support successfully via `ng install sass`', function() {
+ this.timeout(420000);
+
+ sh.exec('npm install node-sass', { silent: true });


I'm not familiar with the project's conventions, but should this be ng install sass ?
(If not, the test description is kind of inaccurate.)

Hi @gkalpak. I need to update the description, you are right. We abandoned the ng install command for now.

@jkuri, I didn't know about ng install 😃
I try to follow along, but you are moving too fast 👍

hansl · 2016-04-04T02:03:33Z

+
+ return new CompassPlugin([compassSrcTree]);
+ } else {
+ return;


This is not necessary.

gkalpak · 2016-04-04T08:07:23Z

+ sass = require('sass');
+ compass = require('compass');
+ } catch (e) {
+ sass = require(`${process.env.PROJECT_ROOT}/node_modules/node-sass`);


If ${process.env.PROJECT_ROOT}/node_modules/node-sass exists, wouldn't require('sass') have picked it up ?

Unfortunately not.

You cannot require peer dependencies without having a peerDependency key in package.json. We could put it there as optional. We need to experiment on this.

jkuri · 2016-04-04T09:47:17Z

Updated, @hansl and @gkalpak please take a look.

gkalpak · 2016-04-04T23:23:03Z

+
+ compile(fileName, inputPath, outputPath) {
+ let sassOptions = {
+ file: path.join(fileName),


path.join --> path.normalize

gkalpak · 2016-04-04T23:27:44Z

I'm not familiar with compass tbh, but I wonder what happens if both node-sass and compass-importer are installed. Won't both SASSPlugin and CompassPlugin be trying to convert those .s[ac]ss files ?
What will happen ? Overwrite each others output ?

hansl · 2016-04-05T17:23:35Z

If you have both installed, one of them will take precendence. If you have a custom workflow you should have a custom broccoli configuration anyway.

@jkuri LGTM.

jkuri · 2016-04-05T17:29:39Z

Thanks Hans.

gkalpak · 2016-04-05T17:59:16Z

But if SASSPlugin always takes precedence over CompassPlugin, doesn't it mean that the latter is effectively not supported ?

jkuri · 2016-04-05T17:59:57Z

@gkalpak I am fixing that.

angular-automatic-lock-bot · 2019-09-09T22:42:04Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

jkuri added the pr_action: review label Apr 2, 2016

jkuri assigned hansl Apr 2, 2016

jkuri added the feature Label used to distinguish feature request from other issues label Apr 2, 2016

gkalpak reviewed Apr 2, 2016
View reviewed changes

jkuri force-pushed the css-support branch from 7f9c41f to bc755e6Compare April 3, 2016 00:03

gkalpak reviewed Apr 3, 2016
View reviewed changes

jkuri force-pushed the css-support branch 2 times, most recently from bbd9a14 to 3fa1f6fCompare April 4, 2016 01:31

hansl reviewed Apr 4, 2016
View reviewed changes

jkuri force-pushed the css-support branch 2 times, most recently from 66545d6 to 19a1762Compare April 4, 2016 04:43

gkalpak reviewed Apr 4, 2016
View reviewed changes

jkuri force-pushed the css-support branch from 19a1762 to f5a8d3aCompare April 4, 2016 09:46

jkuri mentioned this pull request Apr 4, 2016
feat(install): 1. and 3. phase from design docs #294
Closed

gkalpak reviewed Apr 4, 2016
View reviewed changes

hansl added the pr_state: LGTM label Apr 5, 2016

hansl removed the pr_action: review label Apr 5, 2016

jkuri added 6 commits April 5, 2016 19:27

feat(css): broccoli sass plugin
e9cebaa

feat(css): broccoli less plugin
5f7336b

feat(css): broccoli stylus plugin
6a93b3e

feat(css): broccoli compass plugin
f5397c5

test(e2e): add tests for css preprocessors
12a6741

chore: add css preprocessor trees
d2f2404

jkuri force-pushed the css-support branch from f5a8d3a to a8df5ddCompare April 5, 2016 17:28

jkuri force-pushed the css-support branch 2 times, most recently from 21ce9c9 to 3501010Compare April 5, 2016 18:12

docs: css preprocessors support
bf1f762

jkuri force-pushed the css-support branch from 3501010 to bf1f762Compare April 5, 2016 18:54

jkuri merged commit bf1f762 into angular:masterApr 5, 2016

jkuri deleted the css-support branch April 5, 2016 20:26

filipesilva mentioned this pull request Apr 6, 2016
Proposal: integrating efforts #49
Closed

angular-automatic-lock-botBot locked and limited conversation to collaborators Sep 9, 2019

Uh oh!

Conversation

jkuri commented Apr 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkuri commented Apr 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkuri commented Apr 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Apr 4, 2016

Uh oh!

hansl commented Apr 5, 2016

Uh oh!

jkuri commented Apr 5, 2016

Uh oh!

gkalpak commented Apr 5, 2016

Uh oh!

jkuri commented Apr 5, 2016

Uh oh!

angular-automatic-lock-botBot commented Sep 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants