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

Update to angular 11 #874

Merged
merged 5 commits into from
Nov 17, 2020
Merged

Update to angular 11 #874

merged 5 commits into from
Nov 17, 2020

Conversation

sonallux
Copy link
Contributor

@sonallux sonallux commented Nov 16, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features): no need for changes of the tests
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[x] Other... Please describe: Update to Angular 11

What is the current behavior?

Peer dependency warnings when installing the builders in an angular 11 project.

Issue Number: #873

What is the new behavior?

No warnings about peer dependencies. This also fixes the failing dependabot PRs #870 #871 and #872.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Other information

@sonallux sonallux mentioned this pull request Nov 16, 2020
BREAKING CHANGE: update to angular 11
@sonallux sonallux changed the title chore(deps): bump angular-devkit from 10 to 11 WIP: Update to angular 11 Nov 16, 2020
@sonallux sonallux marked this pull request as draft November 16, 2020 12:38
@sonallux sonallux marked this pull request as ready for review November 16, 2020 21:26
@sonallux sonallux changed the title WIP: Update to angular 11 Update to angular 11 Nov 16, 2020
@sonallux
Copy link
Contributor Author

@just-jeb the PR is ready for review.

@just-jeb
Copy link
Owner

@all-contributors please add @jsone-studios for issues and code.

@allcontributors
Copy link
Contributor

@just-jeb

I've put up a pull request to add @jsone-studios! 🎉

Copy link
Owner

@just-jeb just-jeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! I've only had a few questions, please address them and I'll merge.

@@ -7,41 +7,39 @@
"build": "yarn ng build",
"test": "yarn ng test",
"lint": "yarn ng lint",
"e2e": "yarn ng e2e",
"postinstall": "ngcc"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the ngcc postinstall has been removed?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postinstall is no longer recommended (it it faster to process libs that application will import than whole node_modules)
https://github.com/angular/angular/blob/master/CHANGELOG.md#910-2020-03-25

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ngcc produced a warning because of a "solution-style" tsconfig. When I tried to resolve the warning, I found the PR angular/angular#38003 introducing it and found this comment of an angular team member:

Currently our docs do not recommend running ngcc as a postinstall hook, except for a particular scenario related to Angular Universal: https://angular.io/guide/ivy#ivy-and-universalapp-shell.

I tried without the ngcc postinstall and it did work without any issues. Therefore I have removed it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... The ngcc here was rather for example than because of actual need. The thing is that Angular CLI builders (including Karma) use Angular tool chain on order to prepare files for tests, while Jest doesn't. This means that there is no way to run ngcc programmatically only on libraries that you need (because ts-jest doesn't utilize Angular tool chain).
Well, there is a way but it's very complicated and requires major rewrite of ts-jest.
Hence, if your app uses incompatible libs (that were not compiled with Ivy) you have to run ngcc manually on these libs.
I guess we can remove it from the examples and add to the docs, this should be enough.

@@ -7,40 +7,39 @@
"build": "yarn ng build",
"test": "yarn ng test",
"lint": "yarn ng lint",
"e2e": "yarn ng e2e",
"postinstall": "ngcc"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the ngcc postinstall has been removed?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postinstall is no longer recommended (it it faster to process libs that application will import than whole node_modules)
https://github.com/angular/angular/blob/master/CHANGELOG.md#910-2020-03-25

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comment

"jest-preset-angular": "^8.2.1",
"@angular-devkit/architect": ">=0.1100.0 < 0.1200.0",
"@angular-devkit/core": "^11.0.0",
"jest-preset-angular": "^8.3.2",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it closes #854 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does. I have mentioned it in the commit message but forgot to mention it in the PR.

@just-jeb just-jeb merged commit 0db025d into just-jeb:master Nov 17, 2020
@sonallux sonallux deleted the angular-11 branch November 17, 2020 18:28
just-jeb added a commit that referenced this pull request Nov 17, 2020
just-jeb pushed a commit that referenced this pull request Nov 17, 2020
@just-jeb
Copy link
Owner

Sorry guys I'm reverting it for now, webpack-merge update (#819) should be part of this major release since it's a breaking change.
I'll try to update it tomorrow or the day after, need to make sure that all the use cases are covered, since smart strategy has been deprecated.
You're welcome to take a look at this too if you feel like doing it.

@just-jeb
Copy link
Owner

The update actually looks pretty straight forward, but requires some work:

  1. smartStrategy should be replaced with mergeWithRules
  2. Schema has to be updated (also the examples that use strategies)
  3. Docs, tests and migration guide need to be updated as well

@just-jeb
Copy link
Owner

just-jeb commented Nov 17, 2020

Giving it a second thought - I'm gonna release a beta version to let you guys update and will update the webpack-merge afterwards before I release the stable version.
That being said you're still welcome to make a PR in order to speed things up 😄
And thanks again for the great work!

@just-jeb
Copy link
Owner

You can try it now with @next tag.

@kmaraz
Copy link

kmaraz commented Nov 25, 2020

I have tried to install custom webpack builder with @next tag. A few observations:
I needed to install yarn add parse5-htmlparser2-tree-adapter
also yarn add postcss-loader
and after running ng run app:serve I've got this error:

ERROR in ./src/styles.scss
Module build failed (from /home/xxx/projects/app/node_modules/mini-css-extract-plugin/dist/loader.js):
ModuleBuildError: Module build failed (from /home/xxx/projects/app/node_modules/postcss-loader/dist/cjs.js):
ValidationError: Invalid options object. PostCSS Loader has been initialized using an options object that does not match the API schema.
 - options has an unknown property 'plugins'. These properties are valid:
   object { postcssOptions?, execute?, sourceMap? }
    at validate (/home/xxx/projects/app/node_modules/schema-utils/dist/validate.js:104:11)
    at Object.loader (/home/xxx/projects/app/node_modules/postcss-loader/dist/index.js:43:29)
    at /home/xxx/projects/app/node_modules/webpack/lib/NormalModule.js:316:20
    at /home/xxx/projects/app/node_modules/loader-runner/lib/LoaderRunner.js:367:11
    at /home/xxx/projects/app/node_modules/loader-runner/lib/LoaderRunner.js:233:18

ERROR in Module build failed (from /home/xxx/projects/app/node_modules/postcss-loader/dist/cjs.js):
ValidationError: Invalid options object. PostCSS Loader has been initialized using an options object that does not match the API schema.
 - options has an unknown property 'plugins'. These properties are valid:
   object { postcssOptions?, execute?, sourceMap? }
 - options.sourceMap should be a boolean.
   -> Enables/Disables generation of source maps (https://github.com/postcss/postcss-loader#sourcemap)
    at validate (/home/xxx/projects/app/node_modules/schema-utils/dist/validate.js:104:11)
    at Object.loader (/home/xxx/projects/app/node_modules/postcss-loader/dist/index.js:43:29)

@kmaraz
Copy link

kmaraz commented Nov 29, 2020

@just-jeb sorry for notifying you, but I would like to know, if you think this is problem on my part or it should be solved by you. Thank you very much 👍

@just-jeb
Copy link
Owner

just-jeb commented Dec 8, 2020

@kmaraz I'm not familiar with your specific setup but it does work for most of people so I would assume the problem is on your side. Would you mind sharing your angular.json? Do you use experimental Webpack 5 support? Can you try removing package lock and node_module and installing everything from scratch?

@kmaraz
Copy link

kmaraz commented Dec 9, 2020

I have tested it on a different machine and it worked like a charm :) Thank you for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants