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

Switch to AngularCli and remove all Webpack functions #5661

Closed
wants to merge 11 commits into from

Conversation

stevehouel
Copy link
Contributor

@stevehouel stevehouel commented Apr 25, 2017

You can use my sample app for tests : https://github.com/stevehouel/jhipster-angularcli-app

I removed all webpack configuration and dependencies and switch fully on angularly. I added also dedicated environment files for prod and dev configuration

Fix #5624

  • Please make sure the below checklist is followed for Pull Requests.

  • Travis tests are green

  • Tests are added where necessary

  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

You can use my sample app for tests : https://github.com/stevehouel/jhipster-angularcli-app

I removed all webpack configuration and dependencies and switch fully on angularly. I added also dedicated environment files for prod and dev configuration

Fix jhipster#5624
AngularCli already use posts loader in his webpack configuration so we can still use it
@deepu105
Copy link
Member

This would need lot if testing. Im yet to review it in detail but at first look seems like fww things are missing.

  • Browsersync - this is very important in our workflow. Just having live reload wont be enough
  • Sass -unless its automatically handled
  • Notification
  • we had vendor and dev built separate to improve serve performance and to avoid building the heavy vendor part each time
  • copying static assets and swagger stuff

@stevehouel
Copy link
Contributor Author

stevehouel commented Apr 25, 2017 via email

@stevehouel
Copy link
Contributor Author

@deepu105 I found a way to use Browsersync by using lite-server. So one problem solved, I will commit it ASAP.

Regarding other points, I have checked and AngularCli already separated vendor and app build so this is fast when changes are made.

@deepu105
Copy link
Member

deepu105 commented Apr 26, 2017 via email

@jdubois
Copy link
Member

jdubois commented Apr 27, 2017

The build is all red, and indeed there's a lot to do (and test) here, but for me this would be a much better solution than what we currently do. Big +1 then!

@stevehouel
Copy link
Contributor Author

stevehouel commented Apr 27, 2017 via email

@stevehouel
Copy link
Contributor Author

I have done the upgrade for adding browsersync, but there is currently a issue on angular-cli with option --watch on build which is doing an infinite loop and not detecting change anymore, so I will wait for an answer on my issue before commit

@stevehouel
Copy link
Contributor Author

@jdubois I guess that your Travis is incorrect everything looks good into logs and all tests passed, but still in error :(

@pascalgrimaud
Copy link
Member

@stevehouel : something is strange in Travis build, as it is stuck after the yarn test :

.JS 2.1.1 (Linux 0.0.0): Executed 59 of 62 SUCCESS (0 secs / 1.472 secs)
.JS 2.1.1 (Linux 0.0.0): Executed 60 of 62 SUCCESS (0 secs / 1.499 secs)
.JS 2.1.1 (Linux 0.0.0): Executed 61 of 62 SUCCESS (0 secs / 1.531 secs)
.JS 2.1.1 (Linux 0.0.0): Executed 62 of 62 SUCCESS (0 secs / 1.585 secs)

PhantomJS 2.1.1 (Linux 0.0.0): Executed 62 of 62 SUCCESS (1.588 secs / 1.585 secs)
JS 2.1.1 (Linux 0.0.0): Executed 62 of 62 SUCCESS (1.588 secs / 1.585 secs)

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

@stevehouel
Copy link
Contributor Author

Yes @pascalgrimaud I saw it, I am currently checking why ng test is not responding after success

Also reduce test execution to a single shot
@deepu105
Copy link
Member

@jhipster/developers @stevehouel there are few things we have to consider here before we decide on this so I would like to call for a vote from the core team

I'll list some pros and cons of this approach IMO, @stevehouel of course you can add more if you think of any or correct me if Im wrong

Pros

  • Less generated code
  • No need to maintain webpack builds
  • Compatibility issues with angular versions can be minimized

Cons

  • Less control over build (While this might be ok for smaller projects, its not practical for bigger/real projects with lots of other dependencies that might be added) as customizing stuff will be very difficult, like adding new webpack rules for example
  • tight dependency on angular-cli which I personally dont like, so our app breaks if angular-cli changes stuff
  • the learning factor of JHipster is reduced, Many people use JHipster to learn how to do certain stuff so with this they will no longer learn the webpack part
  • We will have to adapt our workflows to workaround what angular-cli does
  • we might loose stuff like webpack notifier, visualizer, reading version from POM
  • We cant independently upgrade typescript, webpack etc

@pascalgrimaud
Copy link
Member

I think about the consistency with the current project with ReactJS, which uses Webpack too.
If one day we decide to integrate React to the main generator, we have to support both, AngularCLI and Webpack, if I'm not wrong.

It will be like Grunt and Gulp, in the past

@jdubois
Copy link
Member

jdubois commented May 2, 2017

First of all, concerning "JHipster React": there is a very high probability we do this. I have a very good client who would be ready to sponsor this, and that would be really awesome. However, I can't guarantee anything yet, and I don't know yet how this will work out technically.

Then there are 3 very important points for me:

  • Using Angular CLI means a lot of people will already know it (all the tutorials and trainings I know are using it), so this will ease the learning curve to get up-to-speed with JHipster, and I think that's very good
  • We still haven't got a good, production-ready Angular 2 build. This is taken us forever, and I'm not even sure anybody works on it anymore at the moment as everyone is fed up with it. So if we can have a "fast lane" to a production build with Angular CLI, then that's the way to go.
  • If we want to have "JHipster React", we also need to have less work on "JHipster Angular". So switching to Angular CLI might remove work for us, and they will also do most of the upgrade work for us, when a new version is released.

I'm seeing this like I see Spring Boot: in the early days JHipster was using Spring directly (that was before Spring Boot existed), and migrating to Spring Boot as removed a lot of headaches for us. And in return, this allowed us to concentrate on more value-added features, like microservices.

Now that looks like I'm a big fan of Angular CLI, but let me be very clear: @deepu105 @sendilkumarn @wmarques all know Angular much better than I do, so they will have the final word on this, of course.

@deepu105
Copy link
Member

deepu105 commented May 2, 2017

@stevehouel I tested this there are some issues I noticed

  • The app UI doesnt seem to load at all. gets a 404 in dev and prod, probably due to files being created inside www/app instead of www. My yo-rc below
  • yarn start starts the browser at http://localhost:3000/ before files are compiled
  • swagger-lib files from node-modules are not copied into the swagger-ui folder
  • html is not optimized in prod mode
  • couldnt test further due to first issue
{
  "generator-jhipster": {
    "promptValues": {
      "packageName": "com.mycompany.myapp",
      "nativeLanguage": "en"
    },
    "jhipsterVersion": "4.3.0",
    "baseName": "jhipster",
    "packageName": "com.mycompany.myapp",
    "packageFolder": "com/mycompany/myapp",
    "serverPort": "8080",
    "authenticationType": "session",
    "hibernateCache": "ehcache",
    "clusteredHttpSession": false,
    "websocket": false,
    "databaseType": "sql",
    "devDatabaseType": "h2Disk",
    "prodDatabaseType": "mysql",
    "searchEngine": false,
    "messageBroker": false,
    "serviceDiscoveryType": false,
    "buildTool": "maven",
    "enableSocialSignIn": false,
    "rememberMeKey": "e7ac761116b381d7adf2a3e1e41e593b260f979c",
    "clientFramework": "angular2",
    "useSass": false,
    "clientPackageManager": "yarn",
    "applicationType": "monolith",
    "testFrameworks": [],
    "jhiPrefix": "jhi",
    "enableTranslation": true,
    "nativeLanguage": "en",
    "languages": [
      "en"
    ]
  }
}

@deepu105
Copy link
Member

deepu105 commented May 2, 2017

Btw jhipster-react will not be an issue as it wont have any code shared with ng2 so that need to be taken into account here

@stevehouel
Copy link
Contributor Author

@deepu105

  • how did you launch your server on prod or dev mode ? using ng serve or yarn start (before my update regarding the browsersync) was launching the app correctly even if the app is deployed on www/app

  • Ok for yarn start, I always had a built version on my target so my mistake. I will revert to my previous launch using the default ng serve. The issue on AngularCli regarding the build with watch option is still there. So Browsersync will be available as soon as they fix it.

  • For swagger-lib, I was not aware of that, I will find a way to do it.

  • Html is not optimized yet on angularcli, but I saw a PR and issues on it, so maybe on a next release.

Copy swagger and shim static files when building app
Revert and remove browsersync till angular-cli issue is not fix
Move target to www
… into angularcli

# Conflicts:
#	generators/client/templates/angular/src/test/javascript/_protractor.conf.js
@stevehouel
Copy link
Contributor Author

So @deepu105, I fix all your issues. I added swagger and shim static files on build and change the target directory to www and not www/app.

@deepu105
Copy link
Member

deepu105 commented May 3, 2017

@stevehouel thanks I will test again today
please do note that we support launching the app in following ways https://jhipster.github.io/profiles/

  • ./mvnw|gradlew, ./mvnw|gradlew -Pprod
  • launch main class file from IDE with dev or prod profile
  • launch the war created from packaging by executing it or using java -jar with dev or prod

@stevehouel
Copy link
Contributor Author

Ok @deepu105 I forgot some case on my tests. I was always using the serve stuff. My bad

HOUEL Steve added 3 commits May 4, 2017 09:57
… into angularcli

# Conflicts:
#	generators/client/templates/angular/_package.json
#	generators/client/templates/angular/webpack/_webpack.common.js
… into angularcli

# Conflicts:
#	generators/client/templates/angular/_package.json
#	generators/client/templates/angular/src/main/webapp/app/_app.constants.ts
#	generators/client/templates/angular/webpack/_webpack.common.js
#	generators/client/templates/angular/webpack/_webpack.dev.js
#	generators/client/templates/angular/webpack/_webpack.prod.js
#	generators/client/templates/angular/webpack/_webpack.vendor.js
@stevehouel
Copy link
Contributor Author

@jdubois & @deepu105 Any news on this PR ?

@jdubois
Copy link
Member

jdubois commented May 12, 2017

I'm sorry I had no time to test.
But just to give more feedback: yesterday I worked a lot with PrimeNG, and if you look at their setup instructions it would have been much easier to integrate if we were using Angular CLI.

@stevehouel
Copy link
Contributor Author

@jdubois I know 👍

@sendilkumarn
Copy link
Member

I will have a look 👍

@sendilkumarn
Copy link
Member

sendilkumarn commented May 14, 2017

Great work @stevehouel

Note: we will be missing webpack more, since for every customisation in the build we have to look forward angular cli to add this and I guess that is the main reason why angular-cli and create-react-app provides eject functionality.

This will reduce our work on one hand but make ourselves depend on angular-cli much more on the other.

@stevehouel
Copy link
Contributor Author

Thx @sendilkumarn.

  • I know that Merge all the translations into a language specific json #5689 break this integration but I notice it before that we need to choose between both and maybe one day we will be fully integrated with angular xi18n et not use anymore ng-translate.
  • Yes that bad 4xxkb files are huge
  • I already fix this and remove that function (waiting that angularcli fix the issue I open). We need to use ng serve by default. For browsersync integration, we will wait a bit.

I understand why webpack is better in a lot of case. Maybe we should add an option for this ? and let the user choose between an optimize version using webpack or an easy integration (but lower perf) using angular-cli. There is not a lot of work to do for be able to switch between both.

@deepu105 & @jdubois what do you think about that option ?

@sendilkumarn
Copy link
Member

If we add an option here to choose between cli & webpack is again we will end up in maintaining two things rather than one which beats the whole point of moving towards cli.

one way is to

  • use angular cli by default
  • use its eject function to move to webpack configuration
  • and then support that webpack configuration with an external module

@deepu105
Copy link
Member

Yes thats the exact reason why we removed Grunt. There is no value in maintaining 2 versions of same thing (Technically angular cli is just webpack stuff underneath)
To me I'm still not in favour of Angular CLI as its not flexible enough and the only advantage is that we have less maintenance burden (which again not so sure if we have to maintain the angular CLI part anyway)
If the concern is that people might expect angular cli commands to work then may be we can try to map the same commands to our webpack build.

@stevehouel
Copy link
Contributor Author

Ok I understand @deepu105 and I agree about having two configuration is not a good option. So maybe what I can do is fix all angular-cli issues on current Master and then be able to remove the ejected option to be fully compatible with both webpack and angular-cli config. But webpack will still be the default option we provide for build/test/package.

@deepu105
Copy link
Member

deepu105 commented May 16, 2017 via email

@stevehouel
Copy link
Contributor Author

If everyone agree my previous proposal, I think you can close this PR. I will create a new one to do that work

@sendilkumarn
Copy link
Member

Thats awesome we do that.

@sendilkumarn
Copy link
Member

let me close this then.

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

Successfully merging this pull request may close these issues.

None yet

5 participants