Add NgModule & build FESM output using ngc & rollup

[tomwayson]

This works npm linked in my local copy of esri-angular-cli-example (master) and in a clean angular-cli app following these instructions.

A few things I noticed when going through this though.

https://github.com/jasonaden/simple-ui-lib and https://github.com/filipesilva/angular-quickstart-lib/blob/master/build.js are actually a bit different. That I've done here is closer to the former, the latter adds minification and UMD. All in all it feels like this stuff is still evolving, and I have ZERO interest in being in that position again.

To use the config from the above, I ended up updating to angular 4. Don't know if it would work w/ 2. Feels like bumping to 4 would require a major version bump of this library. And if we're going to do that... feels like a good time to deal w/ the naming problem that the good folks at angular have left us with (i.e. rename to "angular-esri-loader"). That's a whole other level that I'm not sure I want to get involved w/ right now.

One idea, try rolling this PR back to angular2 libs and see if it works. If so release as a minor bump.

Another idea, wait until https://github.com/filipesilva/angular-quickstart-lib matures, and just fork that, rename as angular-esri-loader, copy over service and module from this, and release as v1.0.0.

[TheKeithStewart]

@tomwayson, I agree that what is considered best practice for structuring Angular libraries is still a bit in flux. One thing to note is that at ng-conf a few weeks back it was said that the Angular team would soon be coming out with a specification for the recommended structure of a library. However, they did not mention a date so "soon" is a relative term here.

Regarding using this in a pre-v4 version of Angular it does appear that there is an issue. When running it the following error is thrown in the browser:

reflection_capabilities.js:61 Uncaught TypeError: ctorParameters.map is not a function
    at ReflectionCapabilities.parameters (http://localhost:4200/main.bundle.js:57533:45)
    at Reflector.parameters (http://localhost:4200/main.bundle.js:57667:44)
    at CompileMetadataResolver.getDependenciesMetadata (http://localhost:4200/main.bundle.js:35299:54)
    at CompileMetadataResolver.getTypeMetadata (http://localhost:4200/main.bundle.js:35264:26)
    at http://localhost:4200/main.bundle.js:35407:41
    at Array.forEach (native)
    at CompileMetadataResolver.getProvidersMetadata (http://localhost:4200/main.bundle.js:35387:19)
    at CompileMetadataResolver.getNgModuleMetadata (http://localhost:4200/main.bundle.js:35146:58)
    at http://localhost:4200/main.bundle.js:35089:50
    at Array.forEach (native)

I'm a bit surprised by this as I'm not aware of any breaking changes around NgModule between v2 and v4. It is possible there the issue lies in a breaking change with the @angular/compiler-cli.

One note though. You have put esri-loader in the peerDependencies. I think that it would be helpful to move this to devDependencies so that it is not necessary to install the esri-loader separately to use this library.

[tomwayson]

Thanks @kgs916

at ng-conf a few weeks back it was said that the Angular team would soon be coming out with a specification for the recommended structure of a library

Isn't that this document?

That appears to be what is informing the repos I used as inspiration for the changes in this PR.

using this in a pre-v4 version of Angular it does appear that there is an issue

Hm.. could be breaking change, or maybe I did something wrong. I'm fine w/ releasing a major version that is 4.0+.

You have put esri-loader in the peerDependencies. I think that it would be helpful to move this to devDependencies

Yeah, I keep going back and forth on this. This time I was doing what the example repos recommended for for the angular libraries themselves, peerDependencies so that the consuming app is not locked to a specific version of esri-loader.

All in all, I don't feel comfortable taking the lead on this since I don't use Angular day to day. If someone who does feels like they've found a pattern that works, I'm glad to accept further commits to this PR or a new PR.

[TheKeithStewart]

@tomwayson, thanks for pointing me to where the Angular library spec is! I guess I missed its actual release.

I'm happy to contribute to the PR and/or take a lead on this issue. It may take me a few days to get on it though. I'll keep you posted.

[tomwayson]

Thanks @kgs916! No rush.

[TheKeithStewart]

@tomwayson I'm starting to put together some changes to this PR. I haven't contributed to an active PR before so we will see how this goes. Looks like since I have already brought down the PR locally I can just create a new commit, push, and that is it 🤞.

Some of the changes that I'm planning to make are:

1. update esri-loader to latest and move to dependencies. I think that this is proper because the other items in peerDependencies (@angular/core and rxjs) are packages that you can realistically expect are already in the projects that will be consuming this but esri-loader likely wont already be there.
2. switch from using rsync in the copy script to using a package that can be added to the devDependencies and used from node_modules/.bin. Currently rsync must be installed globally and on a Windows machine must be manually added into the PATH to get it working.

Let me know if you have any thoughts.

[tomwayson]

@TheKeithStewart great!

Looks like since I have already brought down the PR locally I can just create a new commit, push, and that is it

Ordinarily, yes, but b/c this PR has conflicts, and b/c those conflicts are in package.json and related to "update esri-loader to latest" it's best to first resolve the conflicts. I'll do that, then you can pull this branch again, and you should be good to go w/ the rsync change.

Thanks!

[tomwayson]

Well, this has turned into a real cluster. I've resolved the merge conflicts, but I'm not able to get this to run locally. I'm not sure how my local tests were working before tbh, w/ the way that this is set up to publish from the dist dir. I'm not sure how to npm link from dist to the test apps.

It's late, I'm tired. I've pushed my changes in case you have any insights @TheKeithStewart.

[TheKeithStewart]

The issue is with the copy npm script. There are a number of files from the build folder that are not getting copied into dist. Updating the copy script to this should do the trick:

    "copy": "copyfiles -u 1 -e *.js build/**/* dist && copyfiles -f src/package.json README.md dist",

I tried to make this myself and push the change to this PR but can't seem to figure out how to make it work. I have made this change locally though and tested it out successfully.

Regarding the issue with npm link, there are some issues right now with using npm link with Angular CLI apps. One of the many issues filed about this issue is angular/angular-cli#5428. What I have been doing to get around this is to use npm pack instead and installing the tarball file that it creates in the projects that I want to test.

[tomwayson]

Thx! I'll give this a try later.

[tomwayson]

@TheKeithStewart

First, I was able to npm link from the dist dir - making sloppy errors the other night. However, I still get the Cannot find module 'angular2-esri-loader'. error when trying to compile from my test repo.

I then made the change you suggested to the copy script, re-built, but then get the same error. Still seems like some things are missing from the build output. For example, dist/pacakage.json says "typings": "angular2-esri-loader.d.ts" but that files doesn't exist in my dist folder after running npm run build.

Regarding the issue with npm link, there are some issues right now with using npm link with Angular CLI apps.

More of the perpetual Angular growing pains.

So maybe that's the problem, or maybe I didn't get things set up right. I'm unwilling to publish any changes until I see it working in npm link.

Before I said "If someone who does feels like they've found a pattern that works, I'm glad to accept further commits to this PR or a new PR." However, I'm realizing that I can't even do that. I just don't have the bandwidth to do all the trial and error to figure out what's going on. I'm sorry.

[tomwayson]

@TheKeithStewart FYI - I see the same error after running npm pack in the dist directory and opening the tarball in the node_modules directory of my test app.

[TheKeithStewart]

@tomwayson I still can't seem to figure out how to contribute directly to this PR. What I tried this time was to push a change to the ng-module-FESM-build branch but I get a permission denied error.

I think that the issue we are experiencing has to do with a difference between Mac and Windows and how each OS interprets the string arguments in the copy script. Try updating your copy script with the following:

    "copy": "copyfiles -u 1 \"build/**/*{.d.ts,.js.map,.metadata.json}\" dist && copyfiles -f src/package.json README.md dist",

This works fine on my Windows machine and I believe that the double quotes around the glob path is supposed to do the trick on a Mac. Also, I was having trouble with the exclusion argument so I went to more of an inclusion pattern.

[tomwayson]

My apologies, I thought for sure I had added you as a collaborator earlier, but that was the other repo. You have permission now if you don't mind trying again.

[TheKeithStewart]

No problem, @tomwayson. That worked this time. I have pushed a new commit.

[tomwayson]

@TheKeithStewart - you rock 🎸 🎵 🔈!

This works, npm linked for me.

Was there anything left to do on this or can we ?

[TheKeithStewart]

Awesome!!! I think we are good to go.

Time to 🎉🎉🎉🎉!

[tomwayson]

Merged.

Will release as part of whatever is decided for #15.