Skip to content

Conversation

znck
Copy link
Member

@znck znck commented Apr 3, 2018

No description provided.

@znck
Copy link
Member Author

znck commented Apr 3, 2018

@yyx990803 Please review this. Other style dialect preprocessors would be built in similar fashion.

Copy link
Member

@yyx990803 yyx990803 left a comment

Choose a reason for hiding this comment

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

Looks good except for a few small things.

map: any | null,
options: any
): StylePreprocessorResults {
if (!nodeSass) nodeSass = require("node-sass");
Copy link
Member

Choose a reason for hiding this comment

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

require calls are cached anyway so no need to cache it ourselves here

}

export interface StylePreprocessorResults {
source: string;
Copy link
Member

Choose a reason for hiding this comment

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

results should use code instead of source


export interface StylePreprocessorResults {
source: string;
map?: any;
Copy link
Member

Choose a reason for hiding this comment

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

There are a bunch of semicolons, make sure to stay consistent with the style of the rest of the codebase

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have a style lint config for ts? These semicolons were added by VS code format.

@yyx990803
Copy link
Member

Btw I think we should start writing tests before working on other features.

znck and others added 2 commits April 4, 2018 09:03
- Remove semi-colons
- Use `code` instead of `source` in preprocessor output
@yyx990803 yyx990803 merged commit 9204f16 into vuejs:master Apr 4, 2018
@znck znck deleted the feat/preprocess-style branch April 4, 2018 20:07
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.

2 participants