Fix issues with styles in addon directory #56

[dukex]

I don't know if this approach is the better way to fix the issues, maybe I need understand more about broccoli and ember-cli internals functions, but It's fix the problems(in ember-cli-sass) mentioned on ember-cli/ember-cli#4084 and fix issue #56

For understand why I made this checks please see @stefanpenner anwser on ember-cli/ember-cli#4258

[stefanpenner]

i'll try to make time later this week to review this.

[simonexmachina]

This function should be at the bottom of the file.

[simonexmachina]

I'm okay with this solution, but only if it relies on stable behaviour from ember-cli. What do you think @stefanpenner?

[dukex]

@stefanpenner any feedback?

[stefanpenner]

@stefanpenner any feedback?

haven't had a chance yet

[justin-lau]

@dukex this line is throwing an error when I try to build an application that uses an addon with this branch:

TypeError: Arguments to path.join must be strings
    at Object.posix.join (path.js:488:13)
    at tryFile (node_modules/ember-cli-sass/index.js:43:25)

where root evaluated to { read: [Function], cleanup: [Function] }

[justin-lau]

I have submitted a PR dukex#1 to fix the mentioned issue.

[dukex]

Thanks @justin-lau!

[ahouchens]

Verified that this pull request works perfectly with ember-cli for preprocessing SASS. :) Thanks so much.

[begedin]

Is there anything the consuming app needs to be doing in order for this PR to work?

It works from the dummy app provided by a developing addon, but vendor.css is empty if I consume the developing addon through an external app.

To elaborate:

• Addon some-random-addon is using ember-cli-sass (this branch)
  • Only .scss file I have in the addon is addon/styles/addon.scss
• App my-app is using some-random-addon.
• sass works in dummy app provided by some-random-addon - vendor.css has content
• sass doesn't work in my-app - vendor.css is empty.

Am I missing some part of the solution?

• Does my-app need to depend on ember-cli-sass as well?
• Does my-app need an empty app[.css|.scss]?
• Does some-random-addon need an empty app/styles/app[.css|.scss]?

[justin-lau]

Below is what works for me:

1. some-random-addon should place ember-cli-sass under dependencies instead of devDependencies.
2. If you need Sass in my-app, it should also use this same branch of emebr-cli-sass, but it is okay to place it under devDependencies.

[begedin]

I'm sorry. It was a common mistake on my part. I used npm link to test out the addon and forgot to add it to the list of app's dependencies.

Just in case someone repeats my mistake, the full list of steps to get it working:

• initialize a new addon

ember addon some-addon

• add this branch as a dependency (not devDependency):

cd some-addon
npm install --save git+https://github.com/dukex/ember-cli-sass.git#bugfix/addon-styles

• modify the addon's index.js

// index.js for 'some-addon'
module.exports = {
  name: 'some-addon',
  included: function(app) {
    this._super.included(app);
  }
}

• create addon.scss

mkdir some-addon/addon/styles
touch some-addon/addon/styles/addon.scss

• add whatever it is you want to addon.scss.
• create a new app

cd path/to/parent
ember new my-app

• use npm link to link the addon with the app

cd path/to/some-addon
npm link
cd path/to/my-app
npm link some-addon

• add the addon as a dev dependency for the app, manually, by editing package.json of the app

"devDependencies": {
  ...
  "some-addon": "*"
}

• run the app, it should work now

[ahouchens]

Perfect walkthrough @begedin.

[Fed03]

+1

[dukex]

@justin-lau found I bug in windows, I'll check @justin-lau's pull request and remove the '[WIP]' prefix

[dukex]

@vitch do you got same error without extension options?

[vitch]

Without the extension option when compiling the addons dummy app with this branch I get:

Arguments to path.resolve must be strings
TypeError: Arguments to path.resolve must be strings
    at Object.posix.resolve (path.js:439:13)
    at Object.posix.relative (path.js:505:16)
    at tryFile (/path/to/project/node_modules/ember-cli-sass/index.js:57:17)
    at /path/to/project/node_modules/ember-cli-sass/index.js:26:15
    at Array.reduce (native)
    at SASSPlugin.toTree (/path/to/project/node_modules/ember-cli-sass/index.js:20:48)
    at /path/to/project/node_modules/ember-component-css/node_modules/ember-cli-preprocess-registry/preprocessors.js:186:26
    at Array.forEach (native)
    at processPlugins (/path/to/project/node_modules/ember-component-css/node_modules/ember-cli-preprocess-registry/preprocessors.js:184:11)
    at module.exports.preprocessCss (/path/to/project/node_modules/ember-component-css/node_modules/ember-cli-preprocess-registry/preprocessors.js:154:10)

[dukex]

@vitch I will investigate it
/cc @justin-lau

[sandstrom]

@dukex If this issue is resolved, will that also solve ember-cli/ember-cli#2905?

[dukex]

@vitch I fix it! Update the repository and check again

Sorry delay

[dukex]

@sandstrom I believe not, the ember-cli/ember-cli#2905 example app don't uses ember-cli-sass and this pull request don't has effect in that example

[simonexmachina]

I've gotta say that I think this is really an issue with ember-cli, and it looks that ember-cli/ember-cli#3344 fixes the problem in ember-cli. This PR seems to introduce quite a bit of coupling to ember-cli internals, so I'd prefer to wait until the underlying issue is resolved in ember-cli.

In the meantime addon authors can simply specify the extension option as a workaround.

[ahouchens]

@aexmachina "In the meantime addon authors can simply specify the extension option as a workaround."

That's not accurate. That is another issue which this pull request doesn't address. The only way my files get preprocessed is if I specify the extension option AND point to this PR in my package.json.

[simonexmachina]

Aha, okay thanks for the info. I will reopen and review tomorrow.

On Tue, 11 Aug 2015 23:48 Alex Houchens notifications@github.com wrote:

@aexmachina https://github.com/aexmachina "In the meantime addon
authors can simply specify the extension option as a workaround."

That's not accurate. That is another issue which this pull request doesn't
address. The only way my files get preprocessed is if I specify the
extension option AND point to this PR in my package.json.

—
Reply to this email directly or view it on GitHub
#57 (comment)
.

[ahouchens]

Great thanks @aexmachina!

[simonexmachina]

Double-asterisk ** denotes JSDoc, so this should be a single asterisk.

[simonexmachina]

@stefanpenner any feedback on this? It seems quite tied to implementation details in ember-cli, but I'm happy to go with your advice on this.

[dukex]

I believe the ember-cli/ember-cli#3344 will help us to remove this hack

[simonexmachina]

I've asked @rwjblue for an opinion on whether we should add this hack or wait for the underlying issues to be fixed in ember-cli.

[dukex]

I tried to run ember-cli/ember-cli#3344 but many tests are failed

[alexmngn]

Hi there, when will this pull request supposed be merged into master?

[simonexmachina]

We're trying to avoid merging this, because it's basically a hack to get around a problem in ember-cli which should be fixed when ember-cli/ember-cli#3344 is merged. @stefanpenner should we just merge this and then pull it out when the problem in ember-cli is resolved?

[stefanpenner]

cc @rwjblue whats the status if your fix?

[dukex]

Hi everyone, I made pull request #78 to fix the issue addressed in this pull request, but without hack and using other approach, now I using the #78 in my projects and works fine.

[dukex]

Closing in favor of #78