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

feat(new): include app level import paths for style preprocessors #3724

Closed
wants to merge 1 commit into from

Conversation

admosity
Copy link
Contributor

Fixes #2747 and #3683.

Added a possible solution for sass, less, and I think stylus? (uses paths instead of includePaths).

Added a new configuration property to apps webpackStyleLoaderOptions that only supports one property at the moment includePaths.

Sample Config:

      "webpackStyleLoaderOptions": {
        "includePaths": [
          "node_modules",
          "src/global-css-example"
        ]
      }

Other notable changes:
Removed the plugin webpack.LoaderOptionsPlugin definition in the prod build and added a condition in common for including the css stripping plugin. Seems more DRY.

Updated the schema, but it looks like it generated differently for:

image

Not sure if that's going to be an issue.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@admosity
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@clydin
Copy link
Member

clydin commented Dec 27, 2016

There's already an in progress PR (#2747) for this (with a discuss tag attached and a potentially blocking issue).

@admosity
Copy link
Contributor Author

Yes, except I was looking to move things along. I have referenced the aforementioned PR (#2747) in my comment above. Also in this implementation it provides support for the three major preprocessors that angular-cli supports and not just sass as in the previous PR. And this PR provides an entrypoint through the angular-cli configuration to configure extra paths for any of the preprocessors.

The currently active PR is an implementation on a much stale commit on angular-cli. If anything this can be used as material for the discussion or give pointers for an up to date solution.

That being said... I think there's better ways to handle the loader options (I moved prod and dev into common for the timebeing because of the autoprefixer configuration, seems more DRY). I also think there's a better way to organize the specific logic required per loader to make this happen instead of having a block of code for this specific handling.

@zackarychapple
Copy link
Contributor

@admosity you're going to have to squash the commits. Might as well get that out of the way.

@admosity admosity force-pushed the include-paths-support branch 3 times, most recently from 49d91c8 to 8dd3456 Compare December 30, 2016 09:45
@admosity
Copy link
Contributor Author

Updated to latest master and squashed. I'm a bit slow with the upkeep right now. Currently on vacation.

@admosity admosity force-pushed the include-paths-support branch 2 times, most recently from 406d048 to 3dbf5d2 Compare January 12, 2017 15:16
@admosity admosity force-pushed the include-paths-support branch from 73c6eea to 3dbf5d2 Compare January 12, 2017 19:24
- fixed linter errors
- re-run build-config-interface
- refactored variable names to preprocessors instead of style
- use cliConfig to determine preprocessor for project
@admosity admosity force-pushed the include-paths-support branch from 3dbf5d2 to 446564d Compare January 12, 2017 19:25
@fxck
Copy link

fxck commented Jan 13, 2017

Would this fix sass includePaths in AotPlugin as well? Or the fact that includePaths doesn't work along with AotPlugin is a separate issue?

It's throwing this

ERROR in ./~/@angular/core/src/linker/system_js_ng_module_factory_loader.js
Module not found: Error: Can't resolve '/Users/fxck/www/ngtools-test/$$_gendir' in '/Users/fxck/www/ngtools-test/node_modules/@angular/core/src/linker'
 @ ./~/@angular/core/src/linker/system_js_ng_module_factory_loader.js 69:15-36 85:15-102
 @ ./~/@angular/core/src/linker.js
 @ ./~/@angular/core/src/core.js
 @ ./~/@angular/core/index.js
 @ multi vendors

@filipesilva
Copy link
Contributor

@admosity are you absolutely sure this works for stylus? Because I've been working on this issue as well and as far as I can tell, stylus needs the path options in the loader query and less doesn't work at all.

@fxck I have no idea why it wouldn't work with AoT Plugin. Are you using it separately, with a config that supports paths for sass?

@fxck
Copy link

fxck commented Jan 13, 2017

@filipesilva all I did was

      {
        test: /\.scss$/,
        use: [
          'to-string-loader',
          'css-loader',
          'sass-loader',
          'postcss-loader'
        ],
      },

/// ----

    new LoaderOptionsPlugin({
      debug: true,
      options: {
        sassLoader: function () {
          return {
            includePaths: [ root('src/app/styles') ]
          }
        }
      }
    }),

and then

@import 'variables';

it works fine in JIT, throws

ERROR in ./~/@angular/core/src/linker/system_js_ng_module_factory_loader.js
Module not found: Error: Can't resolve '/Users/fxck/www/ngtools-test/$$_gendir' in '/Users/fxck/www/ngtools-test/node_modules/@angular/core/src/linker'
 @ ./~/@angular/core/src/linker/system_js_ng_module_factory_loader.js 69:15-36 85:15-102
 @ ./~/@angular/core/src/linker.js
 @ ./~/@angular/core/src/core.js
 @ ./~/@angular/core/index.js
 @ multi vendors

when I try to build with AOT

...when I remove the @import, it builds just fine.

AotPlugin is setup this way

    new AotPlugin({
      tsConfigPath: 'tsconfig.json',
      mainPath: root('src/main.browser.ts'),
    }));

@filipesilva
Copy link
Contributor

Superseded by #4003

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants