Skip to content

Migration to Angular 6, RxJs 6 and Webpack 4 #2019

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

Merged
merged 15 commits into from
Jul 9, 2018

Conversation

lanovoy
Copy link
Contributor

@lanovoy lanovoy commented Jun 23, 2018

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Update of configuration and dependencies

  • What is the current behavior? (You can also link to an open issue here)
    Current version is based on Webpack 3, Jasmine 2, RxJs 5.5 and Angular 5.

  • What is the new behavior (if this is a feature change)?

  • Migrated to Webpack 4 with optimized configs
  • Got rid of outdated ngcWebpack (it's pretty much deprecated by the author) - using directly ngtools/webpack
  • Migrated to Angular 6 and RxJs 6.2
  • Added TSLint rules for RxJs 6.2
  • Improved Karma config for local development - should now work similar to Angular/cli based projects
  • Migrated to Jasmine 3

Maksym Lanovyy and others added 12 commits March 16, 2018 14:11
…st and clean:aot with direct rimraf calls to speed up the script execution
* Moved UglifyJsPlugin to proper place
* Enabled proper HMR
* npm run test should work now the same as ng test
* Merge branch 'master' into feature/migrate-to-webpack4

# Conflicts:
#	config/webpack.common.js
#	config/webpack.prod.js
#	package-lock.json
#	package.json
#	yarn.lock
@govi2010
Copy link

looks good to me.

@lanovoy
Copy link
Contributor Author

lanovoy commented Jun 24, 2018

@gdi2290 when you get a chance please review this PR. A big step forward with this PR, so need more people to look into changes here before we merge it.

@lanovoy lanovoy requested a review from PatrickJS June 24, 2018 13:44
…fail for no apparent reason... Have to be investigated.
@envil
Copy link
Contributor

envil commented Jun 25, 2018

Looking forward for this to be merged.

@briosheje
Copy link

Not sure whether I'm the only one experiencing the issue, but updating from the branch to my project, angular seems to be unable to resolve any css whatsoever, neither the ones package-related, nor the inline ones (or included ones).

Was that tested already or am I the only one experiencing that?

@lanovoy
Copy link
Contributor Author

lanovoy commented Jun 25, 2018

@briosheje Maybe you merged something incorrectly? I have the same changes in our production code already and everything seem to be fine. You need to make sure path for mini-css-extract-plugin is set up correctly in your branch (webpack.prod.js)

@briosheje
Copy link

@lanovoy No, sorry, everything was merged properly, the issue was slightly thinner: any css file should be included in styles.scss, I'm using some third-party libraries which were included in polyfills.browsers.ts (official guides explicitly recommended to include these files in *.ts files instead of .scss ones), moving them to scss imports apparently solved the issue, I'm only experiencing some strange icon sizes, but it's not related to the PR itself.

Perhaps it's worth mentioning that with this commit you won't be able to import .scss files in .ts files anymore, or, at least, that it may not work exactly as expected.

@lanovoy
Copy link
Contributor Author

lanovoy commented Jun 25, 2018

Hmm, that's worth investigating more, since future versions of libSass will disallow importing css files using '@import'

@briosheje
Copy link

briosheje commented Jun 25, 2018

@lanovoy the easiest you can try is importing twitter bootstrap, in my case version 4:

package.json:

"bootstrap": "^4.0.0",

If you import through styles.css:

@import '~bootstrap/dist/css/bootstrap.min.css';

It properly works.
If you try to import it through polyfills.browser.ts, it doesn't work:

import 'bootstrap/dist/css/bootstrap.min.css';

It does not throw any error or whatever, it just silently fails with no error (the css seems not to be bundled at all to me, but I might be wrong).

I'm not sure how to investigate further on that topic, I've always had issues with webpack's css resolving with electron some time ago, and literally gave up after hours of guessing. In that case, however, I had no issues at all (until angular 6, of course).

Also, another interesting example is font-awesome: importing it through .scss works in dev mode, but doesn't work in prod mode to me. To avoid issues, I just stopped using font-awesome, but maybe it's somehow related to the css case (about font-awesome, I'm talking about the angular 5 master repository, not this one, I didn't try with this one yet).

@lanovoy
Copy link
Contributor Author

lanovoy commented Jun 28, 2018

@briosheje Did you try to import from app.module or main? polyfill.browser.ts is a separate entry point, so I'm not exactly sure if importing css from it and from the main would be effectively the same.
Also is it only a problem with production builds or is it the same with dev builds?

If you will have time, can you check if projects generated with latest angular cli have a same problem with importing css from ts files?

@briosheje
Copy link

briosheje commented Jun 28, 2018 via email

@briosheje
Copy link

@lanovoy sorry for the late answer, but I had to test some stuff before being able to go on with gfx tests.

Apparently, external libraries does NOT work if they are included either in app.module or in main.browser.

Local scss and css files are fine, but whenever a .scss or css points to anything under node_modules I'm unable to make it work in devmode (not sure in prod mode, but I'm not going to test it).

For instance, I'm including "regular" local .scss and css files in my app.module, while I'm importing third party libraries through scss @import in some .scss files, though it's probably not the best practice.

I will keep you updated if I'm able to find any reason or any workaround to make it work through .ts

@jurisz
Copy link

jurisz commented Jul 4, 2018

When I try to change target output from es5 to es2015 in tsconfig.webpack.json.
I got:
error TS1323: Dynamic import cannot be used when targeting ECMAScript 2015 modules.

@briosheje
Copy link

briosheje commented Jul 4, 2018

@lanovoy Just to keep you updated, both dev and prod mode seems to somehow work by importing css in .scss files, regardless whether they are referenced locally (local css files) or npm dependencies.

I'm still confused about some strange css behaviors in devmode, while in prod mode they seems to be working fine.

I think this pull should just be release asap regardless the .scss issues, perhaps in the near future we may find a way to import .scss directly from .ts files instead (despite there still are so many libraries recommending to import through .scss).

What is interesting, though, and I think this should be mentioned, importing fontawesome now properly works both in dev and prod mode, in my case I'm following this approach in styles.scss:

$fa-font-path: "~font-awesome/fonts" !default;
@import "~font-awesome/scss/font-awesome";

and it's working beautifully, while in the previous version (webpack 3 and angular 5) this was not working in prod mode, so some magic happened there :).

const GTM_API_KEY = process.env.GTM_API_KEY || APP_CONFIG.gtmKey;

const ngcWebpackConfig = buildUtils.ngcWebpackSetup(isProd, METADATA);
const supportES2015 = buildUtils.supportES2015(METADATA.tsConfigPath);

const entry = {
polyfills: './src/polyfills.browser.ts',
main: './src/main.browser.ts'
main: './src/main.browser.ts'
Copy link

Choose a reason for hiding this comment

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

shouldn't these 2 paths start with '../src/' because webpack.common.js is in config folder?

const isProd = options.env === 'production';
const APP_CONFIG = require(process.env.ANGULAR_CONF_FILE || (isProd ? './config.prod.json' : './config.dev.json'));

const METADATA = Object.assign({}, buildUtils.DEFAULT_METADATA,options.metadata || {});
const METADATA = Object.assign({}, buildUtils.DEFAULT_METADATA, options.metadata || {});
const GTM_API_KEY = process.env.GTM_API_KEY || APP_CONFIG.gtmKey;

const ngcWebpackConfig = buildUtils.ngcWebpackSetup(isProd, METADATA);
Copy link

Choose a reason for hiding this comment

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

why don't you delete this line? I thought you replaced ngcwebpack with ngtoolswebpack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The config is still used, though. ngcWebpack is just a wrapper around webpack/ngtools and almost all config values are the same.

Copy link
Contributor Author

@lanovoy lanovoy Jul 9, 2018

Choose a reason for hiding this comment

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

We might want to do some renaming here for clarity...

Copy link

Choose a reason for hiding this comment

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

thanks!

const GTM_API_KEY = process.env.GTM_API_KEY || APP_CONFIG.gtmKey;

const ngcWebpackConfig = buildUtils.ngcWebpackSetup(isProd, METADATA);
const supportES2015 = buildUtils.supportES2015(METADATA.tsConfigPath);

const entry = {
polyfills: './src/polyfills.browser.ts',
main: './src/main.browser.ts'
main: './src/main.browser.ts'
};

Object.assign(ngcWebpackConfig.plugin, {
Copy link

Choose a reason for hiding this comment

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

same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose the path is relative to the folder in which node process for webpack is getting launched from, so that would be the root with package.json - not /config...

@lanovoy lanovoy merged commit 8ba05f7 into master Jul 9, 2018
@klihelp klihelp mentioned this pull request Jul 20, 2018
32 tasks
@PatrickJS PatrickJS deleted the feature/migrate-to-webpack4 branch February 23, 2020 18:26
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.

7 participants