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

(WIP) Implement application decoupling #959

Closed
wants to merge 8 commits into from

Conversation

TheDonDope
Copy link
Contributor

This PR continues #926, pushing the commits to this repositories own branch (issue/913-application-decoupling). It was rebased on the current master. Below is the original description including the last open tasks:

The PR is still a a work in progress:

  • rename src/client to src/browser <- IF browser is now fine for everyone :D I could change it if you want
  • move karma.conf.js to src/browser/karma.conf.js
  • move test-main.js to src/browser/test-main.js
  • move protractor.conf.js to src/browser/protractor.conf.js
  • make sure $ npm start is still running fine (update seed.config.ts)
  • make sure $ npm test is still running fine (updating the paths in karma.start.ts, karma.conf.js and test-main.js
  • exclude karma.conf.js, protractor.conf.js and test-main.js from the copy process to dist/**
  • make sure $ npm run webdriver-start && $ npm run serve.e2e && $ npm run e2e still runs fine should work now with the updates of commit 4b2ca0b
  • implement dedicated package.json in src/browser
  • update build tasks / configuration to create dist/browser/dev|prod|test|tmp directories

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

Have a nice day!

TheDonDope added 5 commits May 26, 2016 11:14
Implements the application decoupling as discussed in #913.
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.
Updates the protractor configuration and associated gulp task.
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.
- Implements the application decoupling as discussed in #913.
- 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.
- Updates the protractor configuration and associated gulp task.
- 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 TheDonDope changed the title Issue/913 application decoupling (WIP) Implement application decoupling Jun 3, 2016
TheDonDope added 3 commits June 3, 2016 15:41
Updates the package.json and gulpfile.ts to fix the issue of running
the e2e.singleRun task
Adds a separate package.json for the sources in src/browser.
@@ -0,0 +1,24 @@
{
"name": "angular2-seed",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need a new name here?

@TheDonDope
Copy link
Contributor Author

Hey guys, i now added the separate package.json in src/browser but i am not quite sure what needs to be in there. If i understood correctly, the package.json in the root folder should only contain the dependencies necessary for the whole build chain, right? And therefore the src/browser/package.json should in turn include those dependencies for the angular2 client?

Looking for input and help here :)

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

@@ -13,8 +13,9 @@
"build.test": "gulp build.test --color",
"build.test.watch": "gulp build.test.watch --color",
"generate.manifest": "gulp generate.manifest --color",
"e2e": "protractor",
"e2e.live": "protractor --elementExplorer",
"e2e": "protractor src/browser/protractor.conf.js",
Copy link
Owner

Choose a reason for hiding this comment

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

The angular dependencies here should be dropped. Doesn't seem required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we then also drop the whole "dependencies": { ... } section in the root package.json, since this has moved to the src/browser/package.json ?

Copy link
Owner

Choose a reason for hiding this comment

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

yeah

@NathanWalker
Copy link
Collaborator

@TheDonDope Curious the latest status here? Do you currently need help or have questions at this point?

@TheDonDope
Copy link
Contributor Author

TheDonDope commented Jun 13, 2016

Hey @NathanWalker,
i'm currently stuck on the refactor of the package.json. I'm not entirely clear on which packages need to be moved to the dedicated src/browser/package.jsonand how this affects the whole "scripts: { ...}" section of the root package.json. Furthermore, since the browser specific node modules will then live within the src/browser directory, we need to update the build tasks and configuration. To be honest i am a little overwhelmed by this and would be glad to get help on that. Basically, the todo list in the description of the PR is still valid:

  • implement dedicated package.json in src/browser
  • update build tasks / configuration to create dist/browser/dev|prod|test|tmp directories

Yet, though those 2 open tasks may sound straightforward, there is a bit more to it than it may sound.

The branch issue/913-application-decoupling is open for further commits :)

Thanks!

@NathanWalker
Copy link
Collaborator

Ok @TheDonDope thanks for update, I'll see if I can take a look at some point this week.

@joshwiens
Copy link
Contributor

@TheDonDope - I'm picking up the last two tasks on the list. I'll PR the changes into the issue/913-application-decoupling branch

@TheDonDope
Copy link
Contributor Author

@d3viant0ne Thank you very much!!!!!!!!

@NathanWalker
Copy link
Collaborator

NathanWalker commented Jun 17, 2016

@mgechev @TheDonDope --

@d3viant0ne and I are working on this together and plan to have a new PR ready here: https://github.com/DeviantJS/angular2-seed/pull/4 and complete for this by Friday night EST (at which time we will close this one in favor of the other), just fyi

@mgechev
Copy link
Owner

mgechev commented Jun 17, 2016

@NathanWalker sounds awesome! Thanks for the help! We're looking forward to merge your work!

@NathanWalker
Copy link
Collaborator

@mgechev Still working through the configuration, need to loosen completion estimate but hoping for this weekend.

@TheDonDope
Copy link
Contributor Author

@d3viant0ne, @NathanWalker: Is there anything i can help you with? I saw there was a merge on your forks develop branch. Thank you so much for your work so far!! :)

Greetings

@NathanWalker
Copy link
Collaborator

@TheDonDope I must report that the decoupling presents problems with SystemJS. More than are likely worth it. We are experimenting in a private project at the moment.
@d3viant0ne You may invite him to the project where further discussions around the experimentations could take place.

@joshwiens
Copy link
Contributor

I honestly just don't see a point in doing it. It's only going to serve to increase both complexity and fragility with at best, limited returns

@TheDonDope
Copy link
Contributor Author

@NathanWalker @d3viant0ne
Hey guys, so do i understand it correctly, that the refactor is not feasible to do due to SystemJS issues? And in consequence the whole refactor is obsolete then?

@joshwiens
Copy link
Contributor

@TheDonDope - I'm sure it's feasible but the implementation adds more complexity than it removes which kind of defeats the purpose.

@mgechev
Copy link
Owner

mgechev commented Jul 4, 2016

@d3viant0ne so after doing research you found out that the introduced complexity is not worth the decoupling that we're going to get?

What is the exact problem with SystemJS that prevents us from finishing the restructuring?

@mgechev mgechev closed this Jul 15, 2016
@TheDonDope TheDonDope deleted the issue/913-application-decoupling branch September 1, 2016 19:41
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.

4 participants