onehilltech / ember-cli-mdc

ember-cli addon for material-components-web
Apache License 2.0
44 stars 15 forks source link

sassOptions.extension = sass gets ignored and app/styles/app.scss: no such file or directory is thrown #36

Open ohcibi opened 3 years ago

ohcibi commented 3 years ago

Note that this issue is different from #4 where the author is using an app.sCss file and not an app.sAss file. I've set sassOptions: { extension: 'sass' } in ember-cli-build.js but still get this error when building my app.

Steps to reproduce

  1. configure sass extension to sass
  2. remove app.scss, add app.sass
  3. build the app

It looks like in this and the following lines: https://github.com/onehilltech/ember-cli-mdc/blob/715b004646336e470ed067db3449d96c2fbe51f5/packages/mdc-sass/lib/sass-compiler.js#L29 the sassOptions passed to the compiler are build manually instead of extending on the user config. Other keys are likely to get lost as well with that.

hilljh82 commented 3 years ago

@ohcibi We are using dart-sass as the sass compiler. Per the documentation, there is no extension option. Can you not rename your sass files to .scss, which seems to be the expected file extension for dart-sass.

ohcibi commented 3 years ago

@hilljh82 Sass files with a .sass extension have a different syntax than those with SCSS. Dart-sass supports both syntaxes, hence '.scss' is not the 'expected' extension for dart-sass, but 'CSS', 'SCSS' and 'SASS'. The very first chapter of said documentation explains both variants.

Mind that parsers/compilers do not care for file extensions. The extension translates to an option for enabling or disabling indented syntax. The CLI decides about it before parsing begins (or better: can begin as the wrong option would result in a syntax error at the end of the very first line of course): https://github.com/sass/dart-sass/blob/a10d7c677dd90f9f5731fa220d1553af620bedea/lib/src/executable/options.dart#L155

In fact, this plugin's dart-sass-plugin module has an ext option which allows for sass and scss obviously, but then .scss is hard-coded all over the place. This is wrong. As mentioned SASS and SCSS are both valid extensions and neither of which is subject to deprecation, so this plugin should support both, see the numerous places in dart-sass implementation that go like extens == css || extension = sass || extension = scss for reference. Moreover, I think sass integration should not be reliant on some UI framework. This plugin should be independent of MDC.

Adding from experiences we learned with ember-cli-sass: The main root cause for errors is the 'feature' to auto import any addon.scss … Here is the extension for every layer of addon/sub-addon needed to be correct. Addon authors should rather add a styles module with the name of the addon and make their blueprints, add that to app.scss if they really want to enforce their styles upon the developer.

ohcibi commented 3 years ago

This is the line where the plugin reads an extension key from the sassOptions but then it is not used: https://github.com/onehilltech/ember-cli-mdc/blob/715b004646336e470ed067db3449d96c2fbe51f5/packages/mdc-sass/lib/sass-plugin.js#L37

hilljh82 commented 3 years ago

@ohcibi First, let's just focus on the issue at hand, which is the extension being recognized within ember-cli-mdc-sass. The other issue you mention is more philosophical.

Would you mind submitting a pull-request to fix the extension problem you are currently having? If the problem goes into issues with the sass compiler itself, which you seem to elude too, then we may have to just table this issue.

ohcibi commented 3 years ago

The issue is that the plugin somehow tries to import an app.scss with scss being hardcoded... i cannot find that place, otherwise I'd had pull requested.. I don't have too much experience with blueprints either, maybe its some magic there.

But i can find quiet a few places where .scss is hardcoded (by grepping). These and other places where the plugin was written with the expectation of only one file extension, need to be changed to support all three possible.

There is a workaround which is to add an app.scss that @uses the real stylesheet with the sass extension. This is a proof that there is no issue at all with dart-sass by the same time.

The issue in its own words is the 'entrypoint' of the sass file. I think it would be better to let the developer define that instead of that extension option.. so rather

sassOptions: {
  entryPoint: 'my-own-file.sass'
}

I haven't looked too much into the api of dart-sass when used from within node. Maybe their option parser can be utilized somehow which would make it a matter only of telling the parser that very name and the rest is handled properlyby dart.. Could also be that a test for sass vs scss is necessary to explicitly set the indented syntax option. (All this applies to the entrypoint only. Since dart knows how to import files properly it is totally valid to blueprint generate an _app-theme.scss or whatever .scss file (which is beyond the scope of this plugin of course). The tricky part is to add the use statement to the users 'app.scss' . Maybe its better to go a less obstrusive approach and generate one material.scss file that is in your control and can therefore get new use statements easily when blueprinting. The developer needs to import that file in whatever way they prefer but manually then

Finally is it worth the hassle? I don't know how far we are with embroider/webpack, maybe that one will handle it in a better way anyways?

As you can see theres quieet a few questions before pring 8-))

hilljh82 commented 3 years ago

@ohcibi Ok, I will go ahead and label this as an enhancement. We are working on updating ember-cli-mdc to support material-components-web 11.0. Maybe we can enhance the ember-cli-mdc-sass add-on to better support what you describe above.

ohcibi commented 3 years ago

Again I suggest to extract the project from the mdc domain. Theres a lot more people interested in sass/scss than only those who want to work with mdc. I wasn't even aware that there is a modern replacement for ember-cli-sass until I got to work with material and this issue's general problem is a known issue from good ol ember-cli-sass and I guess theres some people who have different or even better workarounds/solutions for this.

hilljh82 commented 3 years ago

@ohcibi Personally, I do like having to maintain a separate version of a SASS compiler. The reason we have a separate sass add-on in ember-cli-mdc is the original ones were very slow and did not scale to our needs. Even with the most recent versions, any live update would take up to 2 minutes to process due to the sass build process.

Based on what you state above, this is really no longer a ember-cli-mdc problem/bug. Additionally, it seems that allowing different extension support at the level you are suggesting is outside the scope of this project as well.