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

Make this package implementation-agnostic #186

Merged
merged 2 commits into from
Sep 3, 2018

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Jul 11, 2018

Rather than always loading Node Sass, this now requires users to pass
in either Dart Sass or Node Sass as an option to the plugin.

Closes #184

Rather than always loading Node Sass, this now requires users to pass
in either Dart Sass or Node Sass as an option to the plugin.

Closes adopted-ember-addons#184
Copy link
Contributor Author

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

This pull request isn't complete, but I got as far as I could given my limited knowledge of Ember. I'm not sure how to pass a Sass implementation in to the tests--it doesn't look like there's any actual invocation of new EmberApp() like there is in the README.

2. Ensure you've installed `ember-cli-sass` under `dependencies` in your
`package.json`.
2. Ensure you've installed `ember-cli-sass` and either `sass` or `node-sass`
under `dependencies` in your `package.json`.

3. Define an `included` function in your app:
```js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like we should be encouraging users to pass in a Sass implementation here, but I'm not sure how.

@jbailey4
Copy link
Contributor

@nex3 @aexmachina Is this branch nex3:dart-sass in a workable state allowing consumers to pass dart-sass, it's just tests that are failing?

My team wants to get away from node-sass asap and this is currently the blocker.

If there is anything I can help with let me know...

@jbailey4
Copy link
Contributor

I'm not sure how to pass a Sass implementation in to the tests

@nex3 The tests builds the dummy app (/tests/dummy), could you add the sass implementation option here as well.

@nex3
Copy link
Contributor Author

nex3 commented Aug 31, 2018

@nex3 @aexmachina Is this branch nex3:dart-sass in a workable state allowing consumers to pass dart-sass, it's just tests that are failing?

As far as I know, but I want to emphasize that I'm not an expert in Ember or this package so I can't make any guarantees that I did anything right.

@nex3 The tests builds the dummy app (/tests/dummy), could you add the sass implementation option here as well.

Thanks, done!

@jbailey4
Copy link
Contributor

jbailey4 commented Sep 1, 2018

@aexmachina Looks like the latest change @nex3 made, adding the sass implementation to ember-cli-build.js, got the tests passing! If that's enough to land this, it would be awesome to get this in.

@simonexmachina simonexmachina merged commit ce040a4 into adopted-ember-addons:master Sep 3, 2018
@simonexmachina
Copy link
Collaborator

I think it'd be good for this addon to install sass into the user's package.json. @jbailey4 do you know how to get an addon to do this?

@simonexmachina
Copy link
Collaborator

Published v9.0.0 👍

@jbailey4
Copy link
Contributor

jbailey4 commented Sep 3, 2018

I think it'd be good for this addon to install sass into the user's package.json

@aexmachina We could do this via addPackageToProject in a default blueprint. However, the updated README includes instructions on installing sass or node-sass with this addon. Since other SASS implementations exist and are still supported (e.g. node-sass), it may be better having this addon not assume the implementation.

@simonexmachina
Copy link
Collaborator

However, a lot of people just ember install and expect it to work. I think adding sass to the project is the path of least resistance and offers the least chance of problems. I'm not using this module any more, do you feel like doing a quick PR?

@jbailey4
Copy link
Contributor

jbailey4 commented Sep 4, 2018

I'm not using this module any more, do you feel like doing a quick PR?

@aexmachina Checkout #192

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.

None yet

3 participants