dbfannin / ngx-logger

Angular logger
MIT License
429 stars 110 forks source link

Rework the build system #53

Closed jasekiw closed 6 years ago

jasekiw commented 6 years ago

When working with the library I noticed a few things:

npm link

This one is as easy as removing clean from the build command. Why does the folder need to be cleaned? I haven't thought of a use case for that on build.

Bundling

A rolled up bundle prevents importing parts of the library and breaks tree shaking.

ex. I cannot write the following import

import {HttpService} from 'ngx-logger/adapters/http.service';

here the subfolder is made up. I am just testing to see if I can export one class without referencing the index. I added a class there just to see if it can import it and it can't.

It results in this error.

Module not found: Error: Can't resolve 'ngx-logger/adapters/http.service' in 'C:\Users\Jason\test-ng\src\app'

I rollup build is good for a project that does not have a build system but I think we can assume that if they are running angular then they have a build system like @angular/cli or a similar webpack configuration.

Why remove rollup from the dist buld? It will allow tree shaking. Why should this repository include tree shaking? Now that the package supports a custom http service for the server logging, the user may not want to include the default http service in their bundle.

Unit Tests

There does need to be a bundle system for the unit tests however. It seems like there isn't any karma.conf set up either. I have a branch I am working on right now for setting up karma.


I would like to work on this but I want to ask if there is other input before I write it. @dbfannin

dbfannin commented 6 years ago

The library is build with https://github.com/jvandemo/generator-angular2-library. The way the structure works is the root level handles the building process and all the dev tools, and the actual library code is all under the src directory. When the dist fold gets built, it ignores everything in root, and just builds the src directory.

When doing an npm-link, you must link the src directory not root. Once the source directory is linked follow the barrel pattern of the other services (inside of index.ts, import and export any new services). Once that is done you can simply import {HttpService} from 'ngx-logger';

lastly, I went ahead and set up the testing framework, so if you pull latest from master, it will run successfully.. Now we just need tests! We may need to tweak the setup as we go, but this gives us a starting point. Hope this helps, let me know if I can do anything else. Thanks!

dbfannin commented 6 years ago

I will also add comments to the readme on development

jasekiw commented 6 years ago

@dbfannin I understand that I could npm link the src directory however what is pubished on npm is the contents of the dist directory. By npm-link the dist directory I can test the library as it is actually used in a project. It is ran through rollup which will be bundled again by the user project's bundler.

The dist directory is what I was used npm-link on not the root directory. Sorry for the miscommunication there.

I found this issue on this topic https://github.com/jvandemo/generator-angular2-library/issues/266.

The best solution would be having a build system that builds into the angular package format so that the package can be consumed in the way needed by the user project.

Overall I wouldn't say this is a high priority in any means since the library is fairly small and the tree shaking saving would be very small.

dbfannin commented 6 years ago

@jasekiw, ahh. Sorry, when I originally read this, I misunderstood your intent. I'm not opposed to coming up with a new build process. As long as it is something that doesn't require maintenance. The benefit of using the angular2-library generator, is that they keep it up to date, and pulling in the latest generator code is very easy.

If you have some ideas on how we can improve this, without adding extra maintenance work, I'm all for it.

dbfannin commented 6 years ago

@jasekiw, when we are ready to upgrade the library to angular 6, angular-cli has built in support for angular libraries. I want to go with this approach for refactoring the build system. https://github.com/angular/angular-cli/releases

leo6104 commented 6 years ago

@dbfannin Close this issue?

dbfannin commented 6 years ago

closing!