(WIP) Implement application decoupling #926

TheDonDope · 2016-05-26T09:22:34Z

Hey guys,

i went ahead and started working on implementing the application decoupling as discussed in #913.
The PR is still a a work in progress:

Any feedback/help/change requests are welcome as always :)

Have a nice day!

// cc @mgechev @ludohenin @d3viant0ne @NathanWalker @Shyam-Chen

Implements the application decoupling as discussed in #913.

TheDonDope · 2016-05-26T09:24:25Z

src/browser/karma.conf.js

@@ -47,7 +47,7 @@ module.exports = function(config) {
      // suppress annoying 404 warnings for resources, images, etc.
      { pattern: 'dist/dev/assets/**/*', watched: false, included: false, served: true },

-      'test-main.js'
+      'src/browser/test-main.js'


Currently test-main.js gets copied over to dist/dev/test-main.js. Should that be? Or should it be excluded from the copy task (Don't know right now, which task does the copying)
Question: Should it point to the copied test-main.js in dist/dev or the source test-main.js in src/browser?

Should not be copied because it is not used there. We should use the version in src/browser.

Thanks for the info! I updated the build task accordingly with the commit 0129dac

mgechev · 2016-05-26T09:32:37Z

@TheDonDope awesome job!

Excludes karma.conf.js, protractor.conf.js and test-main.js from the copy build task, so that those files will not be copied over to /dist/** directory. Furthermore also excludes the tsconfig.json from this copy process, as it was copied over before too.

Shyam-Chen · 2016-05-26T09:57:40Z

@TheDonDope

Need to change

https://github.com/mgechev/angular2-seed/blob/master/tools/tasks/seed/e2e.ts#L22
https://github.com/mgechev/angular2-seed/blob/master/tools/tasks/seed/e2e.ts#L25
https://github.com/mgechev/angular2-seed/blob/master/tools/tasks/seed/e2e.ts#L26

TheDonDope · 2016-05-26T10:00:23Z

@Shyam-Chen I already tried that, but it did not have an effect. I think there is another change neccessary in https://github.com/mgechev/angular2-seed/blob/master/package.json#L16

Currently, if you run $ npm run e2e protractor gets called directly, but not through the tools/tasks/seed/e2e.ts.

Shyam-Chen · 2016-05-26T10:02:16Z

@TheDonDope

e2e has two parts, one is CI, the other is a local

Shyam-Chen · 2016-05-26T10:04:43Z

see #837

TheDonDope · 2016-05-26T10:09:21Z

@Shyam-Chen yes thank you i found that one too through the history of package.json.
However on current master it is:

    "e2e": "protractor",

for the local protractor run.

Shouldnt this command needed to be update to something like:

   "e2e": "gulp build.e2e --color && gulp build.js.e2e --color && gulp e2e --color",

Shyam-Chen · 2016-05-26T10:11:42Z

@TheDonDope
Has been corrected.

https://github.com/mgechev/angular2-seed/blob/master/package.json#L30

TheDonDope · 2016-05-26T10:24:13Z

@Shyam-Chen
Haha, thanks! But now i am completely confused.
Currently (on current master) in the package.json there are 4 different protractor related scripts:

  "scripts": {
    [...]
    "e2e": "protractor",
    "e2e.live": "protractor --elementExplorer",
    [...]
    "e2e.ci": "gulp build.prod --color && gulp build.js.e2e --color && gulp e2e --color",
    "tests.all": "npm test && npm run e2e.ci",
    [...]
  },

According to the README.md, if you want to execute the e2e tests locally, you have to:

# e2e (aka. end-to-end, integration) - In three different shell windows
# Make sure you don't have a global instance of Protractor

# npm run webdriver-update <- You will need to run this the first time
npm run webdriver-start
npm run serve.e2e
npm run e2e

# e2e live mode - Protractor interactive mode
# Instead of last command above, you can use:
npm run e2e.live

Is this still correct?

If it is correct, then the gulp script e2e would be triggered, which would then execute protractor directly, which then would fail since the protractor.conf.js has moved to src/browser/protractor.conf.js.

So, since we do have a dedicated gulp task tools/tasks/seed/e2e.ts shouldn't be the "e2e" script be updated? Or even completely removed and only the e2e.ci task be used? (Since it is also included in tests.all).

To sum it up: If i understood everything correctly, my guessed solution to the problem would be the following:

In package.json:

  "scripts": {
    [...]
    "e2e": "gulp build.prod --color && gulp build.js.e2e --color && gulp e2e --color",
    [...]
    "tests.all": "npm test && npm run e2e",
    [...]
  },

Drop the dedicated e2e.ci script and include it into the e2e script. (Which is also used in tests.all then)

What do you think?

Shyam-Chen · 2016-05-26T10:27:39Z

e2e.ci It is a single run for e2e tests.

Shyam-Chen · 2016-05-26T10:28:49Z

rename e2e.ci to e2e.singleRun (IMO)

Updates the protractor configuration and associated gulp task.

TheDonDope · 2016-05-26T10:55:57Z

🎉 Alright, from my side i'm finished, looking forward for a code review :)

mgechev · 2016-05-26T11:14:08Z

LGMT. I'll take a better look in the afternoon and merge.

Guys great job!

Shyam-Chen · 2016-05-26T11:42:59Z

package.json

-    "e2e.live": "protractor --elementExplorer",
+    "e2e": "protractor src/browser/protractor.conf.js",
+    "e2e.live": "protractor src/browser/protractor.conf.js --elementExplorer",
+    "e2e.singleRun": "gulp build.prod --color && gulp build.js.e2e --color && gulp e2e --color",
    "gulp": "gulp",
    "karma": "karma",


Is it going to change?

Hey @Shyam-Chen, what do you mean?

About "e2e" -> I had to add the src/browser/protractror.conf.js to it, since it has been moved to src/browser. The same for "e2e.live".

About "e2e.singleRun" i renamed "e2e.cli" as you suggested to "e2e.singleRun" and sorted it alphabetically under "e2e.live"(since all other scripts are also in alphabetical order)

TheDonDope · 2016-05-26T11:50:55Z

@mgechev Thanks! :) I just wanted to update the README.md to include the new directory structure and discovered that the coverage directory now gets generated within src/browser. I guess that should not be the case, should it? This must have been a side effect of moving the *.conf.js files to src/browser. I'll take another look at it to make sure it gets generated in its original place.
Update: After another look i found that it get generated in the root directory correctly (must have been the leftovers of some previous experiment).

Shyam-Chen · 2016-05-26T12:04:13Z

add new task?

// gulpfile.ts
gulp.task('e2e.singleRun', (done: any) =>
  runSequence('build.prod',
              'build.js.e2e',
              'e2e',
              done));

"e2e.singleRun": "gulp e2e.singleRun --color",  // package.json

Adds a e2e.singleRun gulp task to the gulpfile to leave the script in the package.json more compact. Also updates the README to include the new folder and file structure.

TheDonDope · 2016-05-26T12:17:53Z

@Shyam-Chen Thanks for the suggestion, i updated it and added this with the commit 8ad5161 :)

ludohenin · 2016-05-26T12:18:24Z

Awesome guys.
I think one important bit is missing, package.json ?

TheDonDope · 2016-05-26T12:19:59Z

Hey @ludohenin what about the package.json? What is missing?

TheDonDope · 2016-05-26T12:21:45Z

@ludohenin Oh sh**, i just had another look at the original proposal of #913, and you are totally right! There was a separate package.json proposed inside src/browser. Sorry, i completely forgot about that one!

ludohenin · 2016-05-26T12:21:57Z

In @mgechev's proposal, the client platform app comes with its own package.json file.

TheDonDope · 2016-05-26T12:25:49Z

@ludohenin I guess it will not be as easy as to just transfer the browser app specific dependencies to the src/browser/package.json and then "it just works", would it? Which build scripts would need an update?

TheDonDope · 2016-05-26T12:41:55Z

Also: Would this mean that in consequence that there would be a separete node_modules directory within src/browser? And furthermore: Would there need to be another typings.json within the src/browser directory as well as the corresponding src/browser/typings directory?
And even futher: Would there need to be a dedicated dist/browser/dev|prod|test|tmp output directory? (since there could be potentially multiple applications within src?)
Oh my god i hope this is not a bottomless pit 👻 I think i need a 🍺 now :D

mgechev · 2016-05-26T14:39:41Z

Hahah, I agree about the 🍺, and about the changes that need to be made.

The build needs to be changed to create dist/browser/dev|prod|test|tmp. I think we can live with the current set of typings but package.json is something that needs to be moved to src/browser because otherwise we need to install all the dependencies for all the applications when having many platforms.

TheDonDope · 2016-05-26T15:18:52Z

🚑 Alright, i hereby call for help on the last necessary changes: 🚑

Implementing the dedicated `src/browser/package.json
Updating the necessary build tasks / configuration

Any snippets / commits / fork branches are welcome :)

Thanks guys!

mgechev · 2016-05-27T12:53:43Z

I'll put this in my schedule for the next week/during the weekend.

TheDonDope · 2016-06-03T12:32:18Z

Continued in #959 which has this commits pushed to the issue/913-application-decoupling branch of this repository.

Implement application decoupling

c2e03ed

Implements the application decoupling as discussed in #913.

TheDonDope reviewed May 26, 2016
View reviewed changes

Update protractor configuration

4b2ca0b

Updates the protractor configuration and associated gulp task.

TheDonDope changed the title ~~(WIP) Implement application decoupling (Fix #913)~~ Implement application decoupling May 26, 2016

TheDonDope added the pr: review label May 26, 2016

mgechev self-assigned this May 26, 2016

Shyam-Chen reviewed May 26, 2016
View reviewed changes

Add e2e.singleRun gulp task and update README

8ad5161

Adds a e2e.singleRun gulp task to the gulpfile to leave the script in the package.json more compact. Also updates the README to include the new folder and file structure.

TheDonDope changed the title ~~Implement application decoupling~~ (WIP) Implement application decoupling May 26, 2016

mgechev added work in progress and removed pr: review labels May 27, 2016

This was referenced Jun 2, 2016

Environment configuration management #927

Closed

(WIP) Implement application decoupling #959

Closed

TheDonDope closed this Jun 3, 2016

TheDonDope deleted the issue/913-application-decoupling branch June 3, 2016 12:32

TheDonDope mentioned this pull request Jul 28, 2016

Application decoupling #913

Closed

(WIP) Implement application decoupling #926

(WIP) Implement application decoupling #926

Uh oh!

Conversation

TheDonDope commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheDonDope May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgechev May 26, 2016

Choose a reason for hiding this comment

Uh oh!

TheDonDope May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgechev commented May 26, 2016

Uh oh!

Shyam-Chen commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheDonDope commented May 26, 2016

Uh oh!

Shyam-Chen commented May 26, 2016

Uh oh!

Shyam-Chen commented May 26, 2016

Uh oh!

TheDonDope commented May 26, 2016

Uh oh!

Shyam-Chen commented May 26, 2016

Uh oh!

TheDonDope commented May 26, 2016

Uh oh!

Shyam-Chen commented May 26, 2016

Uh oh!

Shyam-Chen commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheDonDope commented May 26, 2016

Uh oh!

mgechev commented May 26, 2016

Uh oh!

Shyam-Chen May 26, 2016

Choose a reason for hiding this comment

Uh oh!

TheDonDope May 26, 2016

Choose a reason for hiding this comment

Uh oh!

TheDonDope commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shyam-Chen commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheDonDope commented May 26, 2016

Uh oh!

ludohenin commented May 26, 2016

Uh oh!

TheDonDope commented May 26, 2016

Uh oh!

TheDonDope commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ludohenin commented May 26, 2016

Uh oh!

TheDonDope commented May 26, 2016

Uh oh!

TheDonDope commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgechev commented May 26, 2016

Uh oh!

TheDonDope commented May 26, 2016

Uh oh!

mgechev commented May 27, 2016

Uh oh!

TheDonDope commented Jun 3, 2016

Uh oh!

TheDonDope commented May 26, 2016 •

edited

Loading

TheDonDope May 26, 2016 •

edited

Loading

TheDonDope May 26, 2016 •

edited

Loading

Shyam-Chen commented May 26, 2016 •

edited

Loading

Shyam-Chen commented May 26, 2016 •

edited

Loading

TheDonDope commented May 26, 2016 •

edited

Loading

Shyam-Chen commented May 26, 2016 •

edited

Loading

TheDonDope commented May 26, 2016 •

edited

Loading

TheDonDope commented May 26, 2016 •

edited

Loading