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

feat: use packagr to build as angular package format #1634

Merged
1 commit merged into from
Aug 12, 2019

Conversation

terencehonles
Copy link
Contributor

@terencehonles terencehonles commented May 16, 2019

This PR attempts to convert this repo to use ng-packagr to produce builds according to the Angular package format which is needed for AoT builds, and will probably be required as we progress from Angular 7 to beyond.

I tried to leave most things as they were, but the changes I made were the following:

  • Fix imports to be relative within a package, and absolute across packages
  • Simplify how the package.json is templated (only the package.json keys defined in scripts/update-package-json.js are copied from the root into the subpackage package.json)

This does seem to work for us, but there are a couple things that have not been tested or are things to point out:

  • ng-packagr does not load in peer libraries, so in order to allow the build process to be automated I install copy the core library to node_modules after building it and then build the peer libraries. I believe this is related to the issue Feature: dependencies between npm packages ng-packagr/ng-packagr#1174 and there is not a more elegant solution to this. (If there is then that should be done)
  • because of the above point I'm not sure how easy the development flow is, but building from scratch does not seem to take too long.
  • I only tested this when building the package with the Angular compiler. I do not know if the the other bundles are working, and running some tests would help :)

@terencehonles terencehonles force-pushed the use-packagr branch 5 times, most recently from e369cfa to 1b73a7e Compare May 16, 2019 02:34
@codecov
Copy link

codecov bot commented May 16, 2019

Codecov Report

Merging #1634 into master will decrease coverage by 0.03%.
The diff coverage is 7.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1634      +/-   ##
==========================================
- Coverage      40%   39.96%   -0.04%     
==========================================
  Files          41       41              
  Lines        1800     1799       -1     
  Branches      147      154       +7     
==========================================
- Hits          720      719       -1     
  Misses       1079     1079              
  Partials        1        1
Impacted Files Coverage Δ
packages/core/core.module.ts 0% <0%> (ø) ⬆️
packages/core/directives/fit-bounds.ts 0% <0%> (ø) ⬆️
...nazzy-info-window/directives/snazzy-info-window.ts 0% <0%> (ø) ⬆️
...ker-clusterer/services/managers/cluster-manager.ts 54.41% <100%> (-0.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e55daaa...2b1dec4. Read the comment docs.

@ghost
Copy link

ghost commented May 30, 2019

hey @terencehonles . Thanks for sending the PR. Can you please merge the changes from master into your branch? Make sure it's FF only so there are no merge commits.

Thanks in advance!

Repository owner deleted a comment Jun 1, 2019
@terencehonles terencehonles force-pushed the use-packagr branch 4 times, most recently from 979e40b to cd13869 Compare June 2, 2019 21:14
@terencehonles
Copy link
Contributor Author

@doom777 @SebastianM this PR should be updated, but as I noted in the description I only did limited testing in an Angular 7/8 app.

@ghost ghost mentioned this pull request Jul 26, 2019
@ghost ghost requested a review from sebholstein July 26, 2019 20:58
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Good change, and works in my case. However this PR cannot be merge without its follow up, since it takes a bunch of things back like versioning

package.json Outdated Show resolved Hide resolved
@terencehonles
Copy link
Contributor Author

I updated the code to match master. I haven't had a chance to test it, but it does build w/o any warnings.

packages/core/package.json Outdated Show resolved Hide resolved
packages/js-marker-clusterer/package.json Outdated Show resolved Hide resolved
packages/snazzy-info-window/package.json Outdated Show resolved Hide resolved
@terencehonles
Copy link
Contributor Author

I updated some more of the dev dependencies and cleaned up some of the tools I made unnecessary and fixed a test that broke when upgrading versions (I had also fixed it on #1648 ).

I still haven't tested it aside from ensuring it builds, but it should be up to date now.

@terencehonles
Copy link
Contributor Author

terencehonles commented Aug 9, 2019

It does seem the changes still build against the application we have, but I don't have the right data locally to run it so I'll have to test running the actual application elsewhere later.

To test you can use the release or more specifically ngmaps.core-2.tgz (The other was built when I initially drafted this PR)

@terencehonles
Copy link
Contributor Author

terencehonles commented Aug 9, 2019

The changes do still seem to work as before. I don't use any package other than @agm/core and I only use angular w/ aot so if anyone has a different workflow or uses the other packages they should test those.

@terencehonles
Copy link
Contributor Author

Let's not let this get stale again. It wasn't my fault this wasn't merged in earlier. There was some clean up I was able to add because there was more time, but this PR is basically the same as it was in May aside from resolving merge conflicts.

We need to get this into mainline so changes are made with respect to this change rather than me trying to resolve what happened in the meanwhile.

If there is concern about the other builds, we need to add some tests or easy way of verifying they are built correctly. The existing bundle formats that have been moved/updated should be prioritized, and new formats should be fixed as they are noticed to be broken (if they even are)

Just my 2¢

@ghost
Copy link

ghost commented Aug 12, 2019

let's give @SebastianM a few days to review it.

package.json Show resolved Hide resolved
@ghost ghost merged commit df05277 into sebholstein:master Aug 12, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant