just-jeb / angular-builders

Angular build facade extensions (Jest and custom webpack configuration)
MIT License
1.14k stars 198 forks source link

Angular 11 support #908

Closed mbdmardy closed 3 years ago

mbdmardy commented 3 years ago

Hi Guys,

I would like to know your plan or timeline to release the stable version for Angular 11 support? your feedback and update much appreciated. This package really cool so we're planning to utilize it good work!.

Thank you

just-jeb commented 3 years ago

Hi, thanks for your interest. I assume you're asking about @angular-builders/custom-webpack?
If yes, then the blocking PR (#907) has been merge a few days ago and the stable version can actually be released even today.
However I'd like to give it a week or so and see if someone stumbles across some issues with the new mergeRules.
So in short the stable version will be released by the end of this week/beginning of the next week.
It would be really helpful if you upgraded to the latest beta (11.0.0-beta.2) and gave it a try though before the stable version is released.

jeetparikh commented 3 years ago

hey @just-jeb I upgraded to "^11.0.0-beta.3", version and getting this build error

[error] TypeError: Cannot read property 'options' of undefined
    at /Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/index.js:186:51
    at Array.forEach (<anonymous>)
    at /Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/index.js:161:28
    at Array.map (<anonymous>)
    at mergeWithRule (/Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/index.js:138:17)
    at /Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/index.js:211:30
    at Array.forEach (<anonymous>)
    at /Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/index.js:161:28
    at Array.map (<anonymous>)
    at mergeWithRule (/Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/index.js:138:17)
    at customizeArray (/Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/index.js:121:24)
    at _joinArrays (/Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/join-arrays.js:44:50)
    at /Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/merge-with.js:32:17
    at Array.forEach (<anonymous>)
    at mergeTo (/Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/merge-with.js:31:10)
    at /Users/jeetparikh/development/angular-11/node_modules/webpack-merge/dist/merge-with.js:23:15

Also do I need to update custom-webpack to version 11 in order to use angular 11 or the previous version should work out of the box? Thanks

Also, I do not use any merge strategies expliclty defined.. Here is a snippet of my custom config

   module.exports = {
    module: {
        rules: [
            {
                test: /\.scss$|\.sass$/,
                exclude: [/node_modules/, /v1/],
                loaders: [
                    {
                        loader: 'sass-resources-loader',
                        options: {
                            resources: [
                                helpers.root('src/styles/includes.scss')
                            ]
                        }
                    }
                ]
            }
        ]
    },
    plugins: [
        // a couple of plugins
    ]
};

and this is my config in angular.json

"build": {
                    "builder": "@angular-builders/custom-webpack:browser",
                    "options": {
                        "customWebpackConfig": {
                            "path": "./config/webpack-custom.config.js"
                        },
                        "indexTransform": "./config/index-html-transform.js",
jeetparikh commented 3 years ago

An update. beta.1 works fine.. problem starts with beta.2

just-jeb commented 3 years ago

Hey @jeetparikh,

Also do I need to update custom-webpack to version 11 in order to use angular 11 or the previous version should work out of the box? Thanks

Yes, you need the versions to be aligned. If you use custom-webpack@10 with Angular 11 it will cause a lot of problems.

An update. beta.1 works fine.. problem starts with beta.2

Makes very much sense, the breaking change of webpack-merge has been introduced in beta.2.
Could you please create a minimal reproduction repo so that I could take a look? Thanks for your cooperation!

mbdmardy commented 3 years ago

Yes, @just-jeb I'm referring to this package @angular-builders/custom-webpack. That sounds good then and looking forward to this stable release. Thanks for the quick response @just-jeb and keep up the good work. !

jeetparikh commented 3 years ago

hey @just-jeb yep that makes sense. But the fact that I don't use any specific merge strategy or rules.. Just the default version..

just-jeb commented 3 years ago

@jeetparikh Well, default rules are the ones that I defined in order to make it as similar as possible to the smart strategy. It doesn't mean they are right. But in order to check it properly and see if the problem is with the builder or webpack-merge I'm gonna need a repro.

just-jeb commented 3 years ago

@jeetparikh The issue happens because the original config uses use and you use the shortcut for this called loaders.
If you change it to use then the things should get better (still can't vouch that it will work properly, but let's see).
Here is an issue to track in webpack-merge: https://github.com/survivejs/webpack-merge/issues/167.
If switching loaders to use would work for you then we'll have to include it in the migration guide I guess.

jeetparikh commented 3 years ago

hey @just-jeb here is the repo with minimal reproduction of the same use https://github.com/jeetparikh/builder-demo

Tried to use use instead of loaders but still the same error. You can try pulling the latest changes. Also now the repo uses v11.. thanks

just-jeb commented 3 years ago

Right, that's still happening.
You're adding a loader, instead of changing an existing one, so in your case the solution would be passing append rule to use.
Like this:

"mergeRules": {
      "module": {
        "rules": {
          "test": "match",
          "use": "append"
        }
    }
}

I wonder whether I should put this as a default, since this is the most common case. This would mean that only if anyone would want to change options of an existing loader they would have to pass explicit rules.

just-jeb commented 3 years ago

Correction, you have to use the following rules:

"mergeRules": {
      "module": {
        "rules": "append"
    }
}

This is because you have an exclude property, which is different in the original loaders I believe.

renatoaraujoc commented 3 years ago

I'm getting this on beta.3, any ideas?

Captura de Tela 2020-12-16 às 13 43 51

It looks like to be an issue with Tailwind Config:

module.exports = {
  module: {
    rules: [
      {
        test: /\.scss$/,
        use: [
          {
            loader: 'postcss-loader',
            options: {
              postcssOptions: {
                plugins: [
                  require('postcss-import'),
                  require('tailwindcss'),
                  require('autoprefixer'),
                ],
              }
            },
          },
        ],
      },
    ],
  },
};
just-jeb commented 3 years ago

@renatoaraujoc what are your merge rules and what is the expected outcome? Do you want to override the post-css plugins? Append? Could you create a reproduction please? Does using beta.4 change anything?

jeetparikh commented 3 years ago

This is because you have an exclude property, which is different in the original loaders I believe.

hey @just-jeb thanks for the inputs. Does this mean it would still merge my config for my custom rules with the rules provided by angular cli? or append would mean my rules take precedence? Also does the order of rules then matter? Thanks

jeetparikh commented 3 years ago

hey @just-jeb getting the same error as @renatoaraujoc with beta.4 as well and providing no config.. but works if provide this config suggested by you

"mergeRules": {
      "module": {
        "rules": "append"
    }
}

but am not sure of the outcome. Thanks

renatoaraujoc commented 3 years ago

@just-jeb I'm just using the common tailwind css custom web pack config that you can find on the internet (that's the entire file). Where do I put those mergeRules thing? Right now I'm stuck with beta.1 or I can't work...

Renato

axmad22 commented 3 years ago

Painfully I have noticed by experience that Every angular update and they are frequent that the build config format changes....had to update the regret...tested 11.0.0-beta.4 got the same errors like @renatoaraujoc , apparently i missed the memo "mergeRules" whats that a new twist...here my old webpack.config and postcss.config could you please show us the new way of typing in whats suppose to be a simple Config file that would work

thank you


//webpack.config 

const path = require('path');
let sassReader;
try {
  sassReader= require('node-sass');
} catch {
  sassReader= require('sass');
}

module.exports = {
  // Fix for: https://github.com/recharts/recharts/issues/1463
  node: {
  },
  module: {
    rules: [
      {
        test: /\.scss$/i,
        use: [
          'postcss-loader',
          {
            loader: 'sass-loader',
            options: {
              implementation: sassReader,
              sourceMap: false,
              sassOptions: {
                precision: 8
              }
            }
          }
        ]
      },
      {
        test: /styles[\\\/]tailwind\.scss$/,
        use: [
          {
            loader: '@fullhuman/purgecss-loader',
            options: {
              content: [
                path.join(__dirname, 'src/**/*.html'),
                path.join(__dirname, 'src/**/*.ts')
              ],
              defaultExtractor: content => content.match(/[\w-/:]+(?<!:)/g) || []
            }
          },
          'postcss-loader',
          {
            loader: 'sass-loader',
            options: {
              implementation: sassImplementation,
              sourceMap: false,
              sassOptions: {
                precision: 8
              }
            }
          }
        ]
      }
    ]
  }
};
// postcss.config
module.exports = {
  parser: 'postcss-scss',
  plugins: [
    require('tailwindcss')
  ]
}
just-jeb commented 3 years ago

Guys, there is a migration guide I update with each major version.
I invest my time in doing this so I'd expect from you to read it at least.

@renatoaraujoc Another place where you can find "where you put those merge rules thing" is the docs of the builder you're using.

I'm doing my best in order to help you all, but as you can imagine I also have a life and a work and it's physically impossible to debug each one of your configs. I just don't have enough time for this.
Version 11 introduces a breaking change, since the library it uses (webpack-merge) has introduced a breaking change.
I'm trying to keep it as backward compatible as possible (I opened 4 issues there in the last 2 days) but a breaking change is still a breaking change.
This means that you might need to update your builder config. All that's required is to read the docs and understand how it works today.
I'd really appreciate your help, thank you.

just-jeb commented 3 years ago

All: If you don't want to invest any time in reading and understanding how the new auto-merge works please consider using function config. This way you have full control on modifying the original config.

@jeetparikh You still merge your rules, you just define a way it's merged. This config:

"mergeRules": {
      "module": {
        "rules": "append"
    }
}

means that the rules from your config are appended to the rules from the original config.
What does it actually mean? As mentioned in the webpack docs:

Loaders can be chained. Each loader in the chain applies transformations to the processed resource. A chain is executed in reverse order. The first loader passes its result (resource with applied transformations) to the next one, and so forth. Finally, webpack expects JavaScript to be returned by the last loader in the chain.

So if your rule is appended to the rules of the original config, then your rule will actually be executed first for the matching files.
This is also the way it worked before.
When you say you're not sure of the outcome, what do you mean? Is your build producing the expected outcome or not?

mbdmardy commented 3 years ago

Thanks @just-jeb, that merge rules script works for me. I'm testing the loader now if everything works fine.

axmad22 commented 3 years ago

Thanks @just-jeb and sorry did not see the migration guide . We wish you only the best in life, health and in your work which as you can see we cant function without.

just-jeb commented 3 years ago

@axmad22 Huh, thanks for the kind words.

Btw, everyone, one of the simplest things you could do when you stumble across an issue is doing a bit of a debug:

  1. Go to node_modules/@angular-builders/custom-webpack/dist/webpack-config-merger.js
  2. Go into mergeConfigs function
  3. Add some prints of the merged and the resulting configs. For example if you're adding some rules in your config then print the resulting rules and try to see what's wrong.
  4. If you want the test field to be printed properly add this piece of code somewhere in global context (for example next to the mergeConfigs function:
    Object.defineProperty(RegExp.prototype, "toJSON", {
    value: RegExp.prototype.toString
    });

This is exactly what I'm doing with your repro projects, and if you did it in advance and came with the data ready it would help a great deal.

just-jeb commented 3 years ago

@renatoaraujoc There is a bug in merge, for some reason one of the loaders gets screwed in the resulting config:

{
    "test": "/(?:\\.ngfactory\\.js|\\.ngstyle\\.js|\\.tsx?)$/",
    "use": [
      {
        "0": "/",
        "1": "U",
        "2": "s",
        "3": "e",
        "4": "r",
        "5": "s",
        "6": "/",
        ...
      }

I'm looking into it, I suspect it's a problem with webpack-merge.

mbdmardy commented 3 years ago

Hi @just-jeb , Is @renatoaraujoc issue related to using loaders? I build the solution without error using the custom config with loaders. Unfortunately, it seems loaders is not executed because there are no changes in the HTML file with SVG when I used markup-inline-loader. I used this loader in custom webpack image

just-jeb commented 3 years ago

@mbdmardy Yes, it's related to loaders, but I'm still not certain about what happens there yet. If you're not getting the error he does then probably it's not the same problem.
Could you provide a repro for your use case please?

just-jeb commented 3 years ago

@renatoaraujoc There is a bug in merge, for some reason one of the loaders gets screwed in the resulting config:

{
    "test": "/(?:\\.ngfactory\\.js|\\.ngstyle\\.js|\\.tsx?)$/",
    "use": [
      {
        "0": "/",
        "1": "U",
        "2": "s",
        "3": "e",
        "4": "r",
        "5": "s",
        "6": "/",
        ...
      }

I'm looking into it, I suspect it's a problem with webpack-merge.

Indeed an issue with webpack-merge. Here is the one to track, hope it will be resolved soon.

TBH I'm starting to question my decision to upgrade to the new version of webpack-merge.

mbdmardy commented 3 years ago

@just-jeb, I'm trying to transform the img HTML element with markup inline to SVG using loader. I don't have an issue with this config in the build below and I already confirm that this config is merged in mergeConfigs (webpack-config-merger.js). It seems this the loader is not executed /triggered.

Config: "customWebpackConfig": { "path": "./webpack-custom.config.js", "mergeRules": { "module": { "rules": "append" } } },

module.exports = { module: { rules: [ { test: /.html$/, use: [ { loader: 'html-loader', options: { attrs: ['img:src'], ignoreCustomFragments: [/{{.*?}}/], minimize: false, root: resolve(__dirname, 'src'), }, }, { loader: 'markup-inline-loader', options: { svgo: { plugins: [ { removeTitle: true, }, { removeUselessStrokeAndFill: false, }, { removeUnknownsAndDefaults: false, }, ], }, }, }, ], }, ], }, };

This is the goal

image

just-jeb commented 3 years ago

@mbdmardy Try disabling direct templates loading.

mbdmardy commented 3 years ago

Thanks @just-jeb , I'm gonna try those fix.

axmad22 commented 3 years ago

Got to start learning webpack....,but after some reading finally figured I had to add the mergeRules to the angular.json customWebpackConfig section and not in the webpack.config.js >< but Just in case anyone is as blind as me here is what i added

"customWebpackConfig": {
      "path": "./webpack.config.js",
      "mergeRules": {
             "module": {
                  "rules": "append"
               }
         }
 },

It now builds but with some css errors tailwind related but everything is functioning now, also am not sure if it relevant but the option webpack_merge_1.CustomizeRule.Merge, in webpack-merge is undefined. Thanks @just-jeb

elliotleelewis commented 3 years ago

FYI if you use TypeScript 4.1 (using the Angular 11.1 pre-releases), then you get this error message:

$ ng build --prod
✔ Browser application bundle generation complete.

Error: Cannot read property 'flags' of undefined

error Command failed with exit code 1.

Tried the same versions of everything with TS4.0 (while commenting out the lines of my app that rely on TS4.1 bugfixes) and everything compiles correctly. Also with the regular Angular builders (non custom-webpack) and TS4.1, the app builds correctly, so I think the custom-webpack builder needs to be updated to support TS4.1.

EDIT: Here's an example that causes that error: https://github.com/elliotleelewis/portfolio/pull/155/files

jeetparikh commented 3 years ago

I'm doing my best in order to help you all, but as you can imagine I also have a life and a work and it's physically impossible to debug each one of your configs. I just don't have enough time for this.

hey @just-jeb totally understand where you are coming from. You have been totally awesome and super helpful via slack channels and resolving issues. Highly appreciate your efforts. Everyone including myself needs to be more careful and helpful to all OSS contributors as you guys do it in your free time and don't get paid for it. Thanks once again :)

So if your rule is appended to the rules of the original config, then your rule will actually be executed first for the matching files.

Hey @just-jeb thank you for confirming.. with beta4 the output is as expected and I provide this in my config. L get rid of it if you plan to default it

"mergeRules": {
      "module": {
          "rules": "append"
      }
}
renatoaraujoc commented 3 years ago

I'm just awaiting a resolution guys, I'd help fix stuff myself but I hardly know how to webpack... I'm currently using beta.1 so I can build my app without any issues. Totally understand your concerns @just-jeb, you're being awesome dude. My error is the same as @elliotleelewis.

just-jeb commented 3 years ago

@elliotleelewis thanks for bringing it up. I believe the problem is with version mismatch. See, the builder uses the @latest version of Angular Devkit, which means actually latest stable. When you use pre-release version of Angular Devkit (@next) then it may collide with the version the builder uses (which still uses TS 4.0), thus giving you such an error.
This is because the version range that is defined in the buikder dependencies doesn't match rc or next (the version range is >=0.1100.0 < 0.1200.0). If you work with yarn you can use resolutions in order to force the version of the devkit. Otherwise you should probably wait for the stable version of Angular 11.1.
Alternatively, if you can think of a range that would include both >=0.1100 < 0.1200 and pre-release version I would be happy to hear. You can play around with the ranges here.

@renatoaraujoc as I mentioned the issue is with webpack-merge, so I'm waiting for resolution as well. You're welcome to upvote the issue in webpack-merge or even contribute a fix - should be pretty easy.

mbdmardy commented 3 years ago

Hi @just-jeb it works the fix for the loaders awesome!. Everything works fine on my end for the beta 4 version. pls, leave a comment once you release the stable version. your awesome dude! you're the man!

just-jeb commented 3 years ago

@renatoaraujoc I issued a PR that fixes the problem in webpack-merge. Once it's merged I'll be able to provide you with a fixed version.

just-jeb commented 3 years ago

@renatoaraujoc @axmad22 Please try version beta-5 and let me know if it works for you.

just-jeb commented 3 years ago

v11 is officially released.

renatoaraujoc commented 3 years ago

@just-jeb it's working 100%!

Thank you!