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(webpack): Support to add custom/third party css/sccs #1459

Closed
sirajc opened this issue Jul 27, 2016 · 18 comments · Fixed by #1633
Closed

feat(webpack): Support to add custom/third party css/sccs #1459

sirajc opened this issue Jul 27, 2016 · 18 comments · Fixed by #1633
Labels
feature Issue that requests a new feature P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful

Comments

@sirajc
Copy link

sirajc commented Jul 27, 2016

Version/OS Information:

  1. Windows 7
  2. ng --version.
    angular-cli: local (v1.0.0-beta.10, branch: master)
    node: 4.4.7
    os: win32 x64
  1. Repro steps.
    This is CLI App. Have few css files/third party css that needs to be included in the app
import './assets/vendor/paper/bootstrap.min.css';
import './assets/vendor/hint.min.css';

Having above in polyfill.ts or main.ts has no effect, css are not included at all

As a workaround for now, I have included this in index.html

<link rel="stylesheet" href="assets/vendor/paper/bootstrap.min.css" media="screen">
<link rel="stylesheet" href="assets/vendor/hint.min.css" media="screen">

This works fine, but we loose webpack features in this.

Expectation: webpack starters support including css using import, It is expected that cli supports the same behaviour

Important:
I also have app.scss which is common styles for app, due this limitation I cannot include it, for now as a workaround, I have added this in app.component as below

  styleUrls: ['app.component.scss'],
  encapsulation: ViewEncapsulation.None

This way I have app.scss available for whole app, but this is clearly a hack

PS: No error in logs

@aciccarello
Copy link
Contributor

Possibly related #1457

@sirajc
Copy link
Author

sirajc commented Jul 27, 2016

@aciccarello #1457 is about importing files inside your sass file. This is about importing sass/css files in your typescript file. Which will enable webpack to load them in your page.
Also that one is systemjs-brocolli and this is webpack

@filipesilva filipesilva added feature Issue that requests a new feature type: discussion labels Jul 27, 2016
@filipesilva
Copy link
Contributor

filipesilva commented Jul 27, 2016

@sirajc I understand what you are requesting, but disagree with the approach you desire.

Importing css/sass/etc in a code file is a webpack feature, and thus nonstandard. If we were to move to another build system in the future your code would break. As far as the user application is concerned, the CLI is the build system - not webpack. The CLI encapsulates webpack and is providing an abstraction on top of it, and the user shouldn't assume there's webpack there.

It's true that importing .scss files in styleUrls is also nonstandard, but doesn't change the fundamental syntax of the language (TS) and instead stealthily adds sass support to a Angular 2 feature. Not ideal, but not as bad.

Your workaround essentially disables A2 style encapsulation to provide a global stylesheet, which is also not the ideal approach.

The bigger problem to resolve here is importing arbitrary css outside of angular 2. This is needed for global styles and also to prevent the dreaded FOUC (flash of unstyled content).

@sirajc
Copy link
Author

sirajc commented Jul 27, 2016

I agree with the black boxing of webpack, I am not a webpack expert either.
Mapping .scss is webpack specific as well, had to manually update the extension for all components moving from beta.10 to webpack, but that is acceptable
For global stylesheet, I am using ViewEncapsulation.None only for app.component to make it global style, rest all components will have encapsulation available.

Webpack version of CLI is much better, faster and feature rich than earlier and thanks you and the team for that.

Coming to this case, We need to enable some configuration or mechanism to load the global stylesheet and third party stylesheet (Twitter Bootstrap would be mostly required by lots of projects). If not import then something else, but there should be one.
Discussions are open

@TheLarkInn
Copy link
Member

Also related to #1465

@sirajc
Copy link
Author

sirajc commented Jul 28, 2016

@TheLarkInn #1465 talks about creating a module for app like @app, which is like configuring an alias in resolve of webpack config. How will this help in bundling and loading css/scss.

@filipesilva filipesilva added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Jul 29, 2016
@filipesilva
Copy link
Contributor

filipesilva commented Jul 29, 2016

I've been talking with @TheLarkInn to see if we could come up with a overall solution script/link tags, and we came up with something that seems to cover the needed bases in an elegant enough way:

  • specify in angular-cli.json the main app entry points: main.ts, scripts.ts (replaces polyfills.ts), styles.css (can also be sass/etc)
  • have the build system use these entries (currently main.ts and polyfills.ts are hardcoded)
  • on a CSS project, use css imports to load all the other needed CSS files
  • use scripts.ts to load all needed js that would normally go on script tags (users can still add script tags on index.html for stuff hosted on CDNs)
  • move ./public into ./src/public so that relative paths are as expected and we can import files from it with editor support

The key for me is using CSS imports - it's a part of standards and allows webpack to gobble it up on non sass/etc projects. That way all CSS entry points can be handled in the same consistent way.

The CLI team is trying to reach a good solution as fast as possible as this issue is holding up a fair number of people, so please comment on this and other proposed solutions.

@jkuri
Copy link
Contributor

jkuri commented Jul 29, 2016

I opened a PR for this issue: #1492 but I don't know if it is the right solution and it need to be discussed.

My solution generates single app.css file from ./public/**/*.{sass,scss,less,stly} which can be included in index.html.
The pattern to include separate .css file is right as the styles starts loading at the same time as bundle scripts.
The main problem is that the files are not loaded in the right order but the solution can be to have an entry point like app.{sass,scss,less,styl} and all other files can be @import-ed in that entry point. I also think that entry point should be configurable in the cli-config.

Any suggestions and fixes are very welcome.

@sirajc
Copy link
Author

sirajc commented Jul 29, 2016

  • 👍 for having configuration in angular-cli.json for scripts, I was missing vendor.ts and this will help us configure it the way we like
  • I am not sure of browser support for @import in css, but yes it should be trivial with scss/sass
  • instead of moving public we can have src/assets where we can place css, and other external scripts which are not coming from node_modules

Thanks @filipesilva this looks like a good start. Also it would be good if the scripts and compiled css (from sass/scss) also has the hash appended for bursting cache in the prod build.

@filipesilva
Copy link
Contributor

@sirajc browsers wouldn't need support for the native css imports, as webpack would bundle it.

@Meligy
Copy link
Contributor

Meligy commented Jul 29, 2016

@filipesilva (in response to #1459 (comment) )

If you are writing:

import './assets/vendor/paper/bootstrap.min.css';

Inside main.ts, and this is an entry point already, I wonder why this wouldn't work as-is?

Of course assuming you have the right loader for CSS in the webpack config, which raises a different but possibly related question about having a place where the CLI user can override parts of the webpack config, but that's a different issue.

@filipesilva
Copy link
Contributor

@Meligy we don't want to use that pattern (importing .css in .ts) because it's webpack specific instead of a standard.

@clydin
Copy link
Member

clydin commented Jul 30, 2016

@filipesilva I had a similar suggestion (albeit several hours after yours) regarding "global" style configuration (#1492 (comment))

Something to note is that currently css imports for css files are not being processed by web pack (scss files however do work). This concerns styles added to angular components as well.
The loader config is just copying the contents of the css file via the raw-loader.
https://github.com/angular/angular-cli/blob/master/addon/ng2/models/webpack-build-common.ts#L53

Changing the line to something like the following should work:
{ test: /\.css$/, loaders: ['exports?module.exports.toString()', 'css'] },
What this does is process the css file with the css-loader (which handles imports and url()'s) then accesses the raw processed css using the exports-loader transform.

@TheLarkInn
Copy link
Member

No instead when we decide to tackle css styles we will probably take the approach of adding an additional loader targeting a specific pattern to tackle the global CSS.

@clydin
Copy link
Member

clydin commented Jul 30, 2016

I would like to second the suggestion to keep public's semantics to a simple copy to output.

@TheLarkInn Are you responding to my suggested loader change? If so, in either a global or component context, I would consider scss files supporting @import but css files not supporting it, a bug. But i do agree that separate loaders for global css would probably be the way to go.

@xmlking
Copy link

xmlking commented Aug 7, 2016

I was setting sassCompiler options in angular-cli-build.js
so that I can use /node_modules/*** [IDE friendly] instead of node_modules/***
Where should I set the same for angular-cli@webpack?

@import './shared/styles/variables';
@import '/node_modules/bootstrap-sass/assets/stylesheets/bootstrap';
var Angular2App = require('angular-cli/lib/broccoli/angular2-app');
var path = require('path');

module.exports = function(defaults) {
  return new Angular2App(defaults, {
    vendorNpmFiles: [
      'systemjs/dist/system.src.js',
      'zone.js/dist/**/*.+(js|js.map)',
      'core-js/client/shim.min.+(js|js.map)',
      'rxjs/**/*.+(js|js.map)',
      '@angular/**/*.+(js|js.map)',
      'ng2-bootstrap/**/*.js'
    ],
    sassCompiler: {
      cacheExclude: [/\/_[^\/]+$/],
      includePaths: [
        'src/app'
      ],
      importer: function (url) {
        if (url.search('/node_modules') !== -1) {
          return {file: path.join(__dirname,  url)};
        }
      },
      sourceMap: true
    }
  });
};

@filipesilva
Copy link
Contributor

#1633 addresses this issue.

@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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Issue that requests a new feature P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants