URLs in CSS being altered during prod build?

[brian428]

After updating my seed recently, I'm seeing some odd behavior related to relative URLs in some of my CSS files. I'm not sure if this is a bug, but things were fine before I updated to the latest code.

In one of my CSS assets (fontawesome), there are relative URLs like this:
src: url('../fonts/fontawesome-webfont.eot?v=4.6.1');

However, after a prod build, this comes out in my all.css file like this:
src:url(../../../../src/css/font-awesome/fonts/fontawesome-webfont.eot?v=4.6.1)

Was a task or processor added recently that could be altering these relative URLs?

[brian428]

I figured out that it's the rebaseUrls option for gulp-concat-css.

I'll leave this open for now and you can close it if you like. I hate to add yet more options to the build config, but I wonder if it's worth making the options for concatCss() into something that can be overridden in project.config.ts? Since the CSS is aggregated during prod build, it seems fairly easy to get into this situation, where relative URLs in the aggregated CSS become invalid.

[mgechev]

I am not sure if it is worth it to add another configuration option. The task can be overridden, although it seems like an overhead to override it for such a small change. We need to discuss this further but we may also consider to add support for task specific configuration which can be overridden by developers.

// cc @ludohenin

[brian428]

Task-specific config overrides would be nice, but not sure how well that would work. I already have a number of "project-specific" versions of the original seed tasks to add or modify behavior.

So for example, in my project.build.js.dev.ts file, I've set it up to start with a full compile, but then do a typeless compile (using isolatedModules: true) for the next N compiles (N is set in my project config), just to speed things up further. But if a compile error is hit, it keeps doing the full compile again until there are no more errors, and then goes back to the typeless compile loop again. That seemed like a decent compromise between speed and type checking.

I only mention it because I'm not sure how that could be done without actually overriding the whole task. So being able to override some configs in the tasks might cover some scenarios, but there will always be more involved tweaks that will require creating your own task.

[ludohenin]

@brian428 is your configuration issue related to those lines https://github.com/mgechev/angular2-seed/blob/master/tools/tasks/seed/build.html_css.ts#L13-L17 ?

In this task (build.html_css.ts) we have configuration embed which I think isn't such a great idea ... we should probably move this out (L21-L27 as well).

[mgechev]

What I'm a bit bothered is that our seed.config.ts can get even more complex if we include other task related configuration details. On the other hand, I'm not sure if it is worth it to add task-specific configuration, especially given that you can easily override tasks.

The only problem with overriding tasks is that once the seed gets updated you will not take advantage of the latest changes introduced in the overridden tasks. Not sure if this is such a drawback actually because in the end, the goal of the seed is to be a starter for Angular 2 projects and as developers we're supposed to build on top of it.

For this specific case as @ludohenin suggested, we should move the rest of the prefixer's configuration in seed.config.ts because there we already have the BROWSER_LIST there anyway.

[totev]

This bit me yesterday also. Wish I had read this, before losing time debugging everything.
Here is a copy-paste fix for everyone who wants a quick fix and or lands here by some lucky chance.
build.html.css#processExternalCss
.pipe(isProd ? plugins.concatCss(CSS_PROD_BUNDLE,{rebaseUrls:false}) : plugins.util.noop())

[brian428]

@mgechev, I was just about to write this:

At some point a line has to be drawn. It's just not feasible for every config option for every plugin to be exposed in seed.config.ts.

And then realized maybe we actually can! What do you think of using a generic object keyed by plugin to contain any arbitrary plugin configs, and then using a function to look them up in each plugin based on a plugin name key?

Consider the current config setup for browsersync. What if it was done like this instead:

(In seed.config.ts:)

PLUGIN_CONFIGS: any = {
    browserSync: {
        middleware: [require('connect-history-api-fallback')({index: `${this.APP_BASE}index.html`})],
        port: this.PORT,
        startPath: this.APP_BASE,
        server: {
          baseDir: `${this.DIST_DIR}/empty/`,
          routes: {
            [`${this.APP_BASE}${this.APP_DEST}`]: this.APP_DEST,
            [`${this.APP_BASE}node_modules`]: 'node_modules',
            [`${this.APP_BASE.replace(/\/$/,'')}`]: this.APP_DEST
          }
        }
    }
};

And then when it is used in code_change_tools.ts:

let runServer = () => {
  browserSync.init(getConfig('browserSync'));
};

getConfig() would be a helper function in imported from the seed config folder that would try to look up the passed name in the PLUGIN_CONFIGS. If it finds a match, it returns the matching config object for that plugin name. If not, it returns nothing (or an empty object?).

That way:

• Anything that doesn't need a custom config object would just use its defaults.
• Any configs you define in seed.config.ts can be overridden in project.config.ts.
• And finally, any project-specific configs can be added onto PLUGIN_CONFIGS in project.config.ts.

Which means that developers can add on any configs they want to without seed.config.ts turning into a giant list of individual plugin configs.

Edit: And without having to override a whole task just to alter the config object(s) used.

What do you think?

[mgechev]

@brian428 at first glance it looks good to me.

[mgechev]

@brian428 it also might be a good idea to pass a set of default config options to getConfig. For instance:

const DEFAULT_CONFIG = {
  middleware: [require('connect-history-api-fallback')({index: `${this.APP_BASE}index.html`})],
  port: this.PORT
};
let runServer = () => {
  browserSync.init(getConfig('browserSync', DEFAULT_CONFIG));
};

And make the signature of getConfig look like this:

function getConfig(taskName: string, defaultConfig?: object) {
  const config = ...;
  Object.assign(defaultConfig, config);
  ...
}

What do you think? Do you want to take care of the implementation of getConfig and place it somewhere in utils?

[brian428]

I'll see if I can give this a shot in the next day or two.

[brian428]

Actually, just curious: any particular reason why you'd want to pass in the default config as an argument to getConfig(), instead of just having it look up the default config by it's key? My only concern with what you showed is that it means you'd potentially have configs in two places: in standalone variables (like DEFAULT_CONFIG) as well as in the global lookup object (PLUGIN_CONFIGS). Seems like ideally we'd want them all in one place (in PLUGIN_CONFIGS)?

[brian428]

@mgechev Just checking in to see how you want to proceed on this, since ideally the getPluginConfig() check would be used in all applicable plugin calls across all of the tasks. In my case, I selfishly needed to configure the CSS URL rebasing and the BrowserSync config, so those are the ones I added. But there are probably numerous other places where using getPluginConfig() could/should be called.

I can try to take a stab at this, but since I didn't write the tasks and don't have a ton of experience with many of the plugins being used, I'm not sure I'd be the best person to tackle this.

Let me know. I just didn't want it to fall off the radar, since the centralized plugin config lookup is really only useful if it is actually used across all of the tasks.

[TheDonDope]

@brian428 I love the idea to give the tasks some refactoring love! I think it could also be a great opportunity to fix some of those last TODO FIXME markers :) I think we should take some "inventory" on the current tasks and utils to see if we can unify them in some way, maybe extract reusable technical utilities (there are so many tasks that use import 'join' from { path } ...). I will create a tracking issue for that one later tonight.
Thank you for the suggestion @brian428 ! 🍺

[mgechev]

@brian428 I can assist with this. Externalizing the configuration of tasks sounds great. During the weekend I'll use your work for something related, implementing environment specific configuration #927.