Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

make __expression property works in any object #157

Merged

Conversation

bolasblack
Copy link
Contributor

Because sass-loader support custom Sass implementation, and dart-sass implementation is an object, so I hope __expression can works in any object

@bolasblack bolasblack force-pushed the feature/__expression-diffusion-scope branch 2 times, most recently from cde9144 to 0a1b674 Compare March 24, 2019 12:46
@edmorley edmorley added the bug label Mar 24, 2019
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! This looks good to me - @eliperelman do you agree? :-)

Copy link
Member

@eliperelman eliperelman 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, just have a few semantic requests.

README.md Outdated
stringified by setting a special `__expression` property on them:

``` js
const Sass = require('sass')
Copy link
Member

Choose a reason for hiding this comment

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

Can you make Sass lowercase? Also, could you please put semicolons after these lines?

const sass = require('sass');
sass.__expression = `require('sass')`;

// ...
.use(MyPlugin, [{ fn: myFunction, implementation: sass, }]);

test/Config.js Outdated
@@ -524,6 +524,10 @@ test('toString with custom prefix', t => {

test('static Config.toString', t => {
const config = new Config();
const Sass = {
Copy link
Member

Choose a reason for hiding this comment

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

Use lowercase sass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌 now

@bolasblack bolasblack force-pushed the feature/__expression-diffusion-scope branch from 0a1b674 to 65fa28e Compare March 25, 2019 04:49
Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Looks great!

@eliperelman eliperelman merged commit ff4ef28 into neutrinojs:master Mar 25, 2019
@bolasblack bolasblack deleted the feature/__expression-diffusion-scope branch March 25, 2019 16:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants