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

fix(publish): emit type declarations with CJS build #4708

Closed
wants to merge 1 commit into from

Conversation

jeffbcross
Copy link
Contributor

Closes #4706

@jeffbcross jeffbcross added type: bug/fix comp: build/pipeline action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 13, 2015
@jeffbcross
Copy link
Contributor Author

The problem was that our npm publish script is copying output from dist/js/cjs, which is generated from the Node broccoli tree, instead of from dist/js/prod/es5, where the typings were correctly generated from our broccoli build.

I fixed this by changing the Node tree to emit type declarations, which seems like a safe change (with hopefully very little latency added to our cjs build). The other way I could have fixed this would have been to change the npm_publish script to copy files from dist/js/prod/es5 instead of dist/js/cjs, but I'm unsure of the impact of that change since there are many subtle differences between the two outputs. For example, the prod output of angular2.js includes import "./manual_typings/globals.d.ts"; which the node/cjs output does not include.

Here is a screenshot of the diff of the npm folder before making this change (left), and after making this change (right). Notice the lovely d.ts files included on the right.

screen shot 2015-10-13 at 2 46 10 pm

@vsavkin vsavkin added pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 13, 2015
@jeffbcross
Copy link
Contributor Author

@alexeagle and I are going to make a couple more changes to the Node Tree to get the output we need.

  • Add manual_typings to the output
  • Add a step in the Node tree to rewrite angular.d.ts to include an import to manual_typings/globals.d.ts.

@jeffbcross jeffbcross added the action: merge The PR is ready for merge by the caretaker label Oct 13, 2015
@mary-poppins
Copy link

Merging PR #4708 on behalf of @jeffbcross to branch presubmit-jeffbcross-pr-4708.

@mary-poppins mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Oct 13, 2015
@alexeagle
Copy link
Contributor

Reviewed in person. Thanks Jeff!!

On Tue, Oct 13, 2015 at 3:16 PM Mary Poppins notifications@github.com
wrote:

Merging PR #4708 #4708 on behalf
of @jeffbcross https://github.com/jeffbcross to branch
presubmit-jeffbcross-pr-4708
https://github.com/angular/angular/tree/presubmit-jeffbcross-pr-4708.


Reply to this email directly or view it on GitHub
#4708 (comment).

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

individual d.ts files not included in npm distribution
5 participants