Skip to content
This repository was archived by the owner on Jun 23, 2025. It is now read-only.

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

Merged
1 commit merged into
sebholstein:masterfrom
terencehonles:use-packagr
Aug 12, 2019
Merged

feat: use packagr to build as angular package format#1634
1 commit merged into
sebholstein:masterfrom
terencehonles:use-packagr

Conversation

@terencehonles
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

@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

Comment thread package.json Outdated
@terencehonles
Copy link
Copy Markdown
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.

Comment thread packages/core/package.json Outdated
Comment thread packages/js-marker-clusterer/package.json Outdated
Comment thread packages/snazzy-info-window/package.json Outdated
@terencehonles
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

ghost commented Aug 12, 2019

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

Comment thread package.json
@ghost ghost merged commit df05277 into sebholstein:master Aug 12, 2019
This pull request was closed.
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.

1 participant