Skip to content

git feat(serve): add option to serve app from dist folder #4975

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

Closed
wants to merge 1 commit into from

Conversation

itslenny
Copy link
Contributor

@itslenny itslenny commented Feb 24, 2017

Add a flag to allow serving the app from the dist folder.

This allows users to run ng build and then run ng serve --use-dist to serve from the dist folder. This is also enabled for ng e2e which allows running ng build with the appropriate environment / target and then running e2e tests against the build (without ng serve rebuilding). This is especially useful for continuous deployment scenarios.

This should resolve: #4293, #4722, and #4830

@itslenny
Copy link
Contributor Author

I would also like to enable running ng test against the dist build, but that is a bit more complicated so I'll try to figure it out and submit it as a separate PR if this is accepted.

Add a flag to allow serving the app from the dist folder.
@@ -14,6 +14,7 @@ export interface E2eTaskOptions extends ServeTaskOptions {
webdriverUpdate: boolean;
specs: string[];
elementExplorer: boolean;
useDist: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

dist has no special meaning in the CLI terms. It's called outDir in the config, and outputPath in the build command arguments.

Copy link
Contributor Author

@itslenny itslenny Feb 24, 2017

Choose a reason for hiding this comment

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

@Meligy, I questioned calling it this, but I wasn't sure what the right naming would be. I want something that says "use existing build" but I'm not sure the best way to express that. Should it just be --use-output-path (feels a bit verbose to me). Any ideas?

const contentBase = path.resolve(this.project.root, outputPath);
if (serveTaskOptions.useDist) {
if(!fs.existsSync(contentBase)) {
throw new SilentError('Dist directory not found');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we even care here? We were going to delete it anyway.

Copy link
Contributor Author

@itslenny itslenny Feb 24, 2017

Choose a reason for hiding this comment

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

No we're not. This flag serves data from the existing dist folder if it exists. If it doesn't exist that's not possible so we need to throw an error.

If the flag isn't used then it behaves the same as before (Deletes the dist folder and serves from memory)

Copy link
Contributor

@Meligy Meligy Feb 25, 2017

Choose a reason for hiding this comment

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

build deletes the folder if it exists and then creates it again. It'd make sense for serve to act the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the point of this is to serve whatever got built in build, I think this would be explicitly outside of the purpose of this command.

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 the point of this is exactly to serve a previous build. It allows the user the option to build using whatever options they want with the files written to disk and serve that. More importantly, it allows running e2e's against a build for use on a build machine / continuous deployment. Today to deploy using ng-cli I'm building the site 3 times.

  • ng test builds and then tests
  • ng e2e runs ng serve and e2e's
  • ng build to create the actual bits for deployment

I'd rather use ng build first to build EXACTLY what will be deployed then run the unit tests and e2e tests against what will be deployed. This is a pretty standard thing to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather use ng build first to build EXACTLY what will be deployed then run the unit tests and e2e tests against what will be deployed. This is a pretty standard thing to do.

Good point.

At the moment the only way to do this is to run e2e with the same build options as the ones passed to get files for distribution (since serve inherits build options, and e2e inherits serve options).

@clydin
Copy link
Member

clydin commented Feb 25, 2017

Is there an actual use case for the ng serve addition?
While is works, it is essentially a hack.
For e2e it would make more sense (and be far cleaner), to just use express directly to serve a specified directory.
However, a user could also achieve this currently by using an existing simple http server (http-server for instance) and running e2e with --no-serve.

@itslenny
Copy link
Contributor Author

itslenny commented Feb 25, 2017

@clydin mostly it is for the purpose of running e2e's for continuous deployment. However, it could also be used for (admittedly rare) debugging purposes.

Personally, I think running your own server sounds more like a hack. IMO being able to run my e2e tests against the build that I plan to deploy seems pretty standard, and should be something ng-cli can do directly without me needing additional scripts / dependencies.

Today I have to build 3 times to do continuous deployment (test and e2e both build). Then, I build for the actual deployment which SHOULD be the same if I use the same flags, but aside from speed I'd feel a lot safer if I could run the tests against the actual code that I'm about to deploy.

Additionally, my plan is to eventually implement this for ng test in a separate PR so ng-cli can support continuous deployment directly without needing to build multiple times. The build flow would be ng build ... ng test ... ng e2e all running off the original build.

@clydin
Copy link
Member

clydin commented Feb 25, 2017

The hack is the creative use of the contentBase and lazy options. This is not their intended use and there is no guarantee that their internal operation will remain the same moving forward. It also initializes a large amount of infrastructure (webpack, etc.) to essentially serve a handful of static files.

If your goal is to validate a production deployment, then running the e2e tests against a development server is probably not the most appropriate course of action anyway. A staging server with a production representative configuration and environment would be a far better alternative.
Also, the unit testing by definition requires separate compilation. It's testing the internal components of the application in scenarios that may or may not be triggered externally and at a much lower level.

@itslenny
Copy link
Contributor Author

Webpack server is really light I wouldn't call it a large amount of infrastructure. However, spinning up an entire staging environment and deploying the site there is. As you said, it's just s simple static http server using the dev server vs the actual production environment should have no impact.

a unit test by definition requires separate compilation

I don't follow what you mean by this. Why would that be necessary?

@itslenny
Copy link
Contributor Author

itslenny commented Mar 1, 2017

Interestingly, I just hit my first bug related to this today. When you run ng serve (or ng e2e) it serves any file in the root of src/ which means if you put some sort of static asset in src (e.g. /src/myfile.json) it will work fine with ng serve AND it will also pass on ng e2e even if you use the -prod flag. HOWEVER, when you deploy it will crash at runtime because that file won't be available in the dist folder.

Yes, I realize I can add additional assets into the angular-cli.json, but the point stands that due to the fact that e2e's use ng serve instead of allowing you to use the dist folder allowed a broken version of my app to deploy today this could've been avoided with this PR checked in.

@filipesilva
Copy link
Contributor

I agree that this is an interesting idea, and I actually tried to do something similar before...

I settled on allowing --no-watch for serve instead for now.

Part of it was bugs like the one you just ran into. The dev-server hack has very real pitfalls.

There is also the very real consideration that there are several other settings that you can't reproduce this way, like --base-href and --deploy-url.

Then there is the motivation for this functionality itself... if you want to be sure the build actually works and you don't trust the in-memory one, why would you trust running ng serve --use-dist instead? It's still blackboxed. Whatever you feared before, you would still fear now.

I don't question the usecase, mind you. I talked about it a lot with @johnpapa.

But I question the design, implementation and perception.

So this PR as presented is not viable. Can I ask you to open a new issue to discuss this instead? Please tag me and @johnpapa as we're both quite interested in this topic.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants