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

improve package bundling #84

Merged
merged 2 commits into from
Nov 8, 2017
Merged

Conversation

e-cloud
Copy link
Contributor

@e-cloud e-cloud commented Nov 7, 2017

previously, ngx-pipes uses a simple bundle tool chain to publish no-clean-enough package, it raises some resolve conflicts with angular-cli and latest angular. Here we apply the ng-packagr to generate the bundles following the Angular package format. With that, the output could be easily consume by various module loader.

Breaking Change:

  • here we flatten one folder with app folder removed, if someone used to use the code or types from source, you have to adapt the path.
  • we do not publish the source file of each pipe now. As with webpack/rollup's treeshaking feature, we don't need to publish pipes separately. User who used to reference to source pipes directly should update their paths.

The PR should only be merged after PR #83.

@e-cloud e-cloud force-pushed the feature/bundle-tool branch 2 times, most recently from d280d26 to 4d05c12 Compare November 8, 2017 07:21
Breacking change: here we flatten one folder with app folder removed, if someone used to use the code or types from source, you have to adapt the path.
previously, ngx-pipes uses a simple bundle tool chain to publish no-clean-enough package, it raises some resolve conflicts with angular-cli and latest angular<Angular v5>. Here we apply the ng-packagr to generate the bundles following the [Angular package format](https://docs.google.com/document/d/1CZC2rcpxffTDfRDs6p1cfbmKNLA6x5O-NtkJglDaBVs/edit). With that, the output could be easily consume by various module loader.

Breaking Change: we do not publish the source file of each pipe now. As with webpack/rollup's treeshaking feature, we don't need to publish pipes separately. User who used to reference to source pipes directly should update their paths.
Copy link
Owner

@danrevah danrevah left a comment

Choose a reason for hiding this comment

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

Looks good.. breaking changes will require a version bump from 1 to 2.

package.json Outdated
"standard-version": "^4.2.0",
"ts-node": "^3.0.0",
"tslint": "^5.5.0",
"typescript": "~2.5.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Lets upgrade this to the latest 2.6.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danrevah I'm sorry, i forgot to fix this version. we'd better not upgrade the typescript bigger than 2.4. Because Angular doesn't support it now. I've tried to upgrade to v2.5 in another package, with rollup, it produces incorrect output. So, upgrading typescript is not proposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you have to downgrade it, like typescript: ~2.4.0

Copy link
Owner

@danrevah danrevah Nov 8, 2017

Choose a reason for hiding this comment

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

It's 2.4.0 in the merge, and this commit has been merged and released in version v2.0.0.

Thanks!

@danrevah danrevah merged commit 6a6f8ca into danrevah:master Nov 8, 2017
@e-cloud e-cloud deleted the feature/bundle-tool branch November 8, 2017 09:12
@danrevah
Copy link
Owner

danrevah commented Nov 8, 2017

That was a really great work.

Thank you for your help in improving this library!

@danrevah
Copy link
Owner

danrevah commented Nov 8, 2017

this commit has ruined the AOT compilation #86 .. I will try to investigate, do you have any leads?

@danrevah
Copy link
Owner

danrevah commented Nov 8, 2017

Ok this was a really easy fix, I saw that the metadata files are not being generated properly.

For reference this is the commit that should fix it:
4b001cc

@e-cloud
Copy link
Contributor Author

e-cloud commented Nov 11, 2017

@danrevah sorry for that bug, glad you have fixed it.

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.

2 participants