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 closure imports by appending /index where needed #13471

Closed
wants to merge 4 commits into from

Conversation

alexeagle
Copy link
Contributor

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[ ] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@alexeagle alexeagle added the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 14, 2016
@alexeagle alexeagle requested a review from mhevery December 14, 2016 19:42
@alexeagle alexeagle added pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 14, 2016
@alexeagle
Copy link
Contributor Author

Hmm, this is currently blocked.

The issue is that tsickle now rewrites import {} from '@angular/core' to import {} from '@angular/core/index'. This is in the ES5 JS sources. Then when we bundle these for UMD, we get eg. compiler/bundles/compiler.umd.js containing:

(function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('@angular/core/index')) :
    typeof define === 'function' && define.amd ? define(['exports', '@angular/core/index'], factory) :
    (factory((global.ng = global.ng || {}, global.ng.compiler = global.ng.compiler || {}),global._angular_core_index));
}(this, function (exports,_angular_core_index) { 'use strict';

here, the deep import from @angular/core/index causes the loader to skip the UMD bundle for core, and instead load ES2015 module syntax from index.js.

@alexeagle alexeagle force-pushed the closure branch 5 times, most recently from 87d994f to f9d1fe9 Compare December 17, 2016 02:45
@alexeagle
Copy link
Contributor Author

This is now unblocked, @IgorMinar you should probably take a look since this adds the experimental ES6 build (guarded by an env variable)

@alexeagle alexeagle force-pushed the closure branch 2 times, most recently from ce4372e to 9a2ce44 Compare December 19, 2016 21:27
@IgorMinar
Copy link
Contributor

IgorMinar commented Dec 21, 2016

We should update the Angular Package Format doc before merging these changes to truly understand the impact. I read through the PR but I'll need to checkout the branch and actually build it to see what are all the changes.

Let's not merge this until the design doc is updated. I'll work with Alex on this once he's back from vacation. I believe that this is PR is not super urgent and can wait a few days. If not please let me know.

@alexeagle
Copy link
Contributor Author

This PR shouldn't change the existing build, because the features are guarded by a flag.
It's okay to wait until second week of January. Just some rebasing sadness, hopefully nothing big landing in the meantime...

@alexeagle
Copy link
Contributor Author

PTAL

@IgorMinar
Copy link
Contributor

IgorMinar commented Jan 17, 2017

please fix commit messages:

  • chore(tsc-wrapped) -> build(tsc-wrapped)
  • feature(tsc-wrapped) -> feat(tsc-warpped) x 2
  • feat(build): -> build: ....


let {output, externs, diagnostics} =
tsickle.annotate(this.oldProgram, sourceFile, {untyped: true});
const es2015Target = this.options.target == ts.ScriptTarget.ES2015; // This covers ES6 too
Copy link
Contributor

Choose a reason for hiding this comment

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

covers? it's equivalent. no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

meaning this conditional matches if you pass --target=ES6 on the command line.
ES6 and ES2015 get the same integer value

@@ -131,6 +132,13 @@ do

echo "====== COMPILING: ${TSC} -p ${SRCDIR}/tsconfig-build.json ====="
$TSC -p ${SRCDIR}/tsconfig-build.json
# ES2015 distro is not ready yet; don't slow down all builds for it
# TODO(alexeagle,igorminar): figure out ES2015 story and enable
Copy link
Contributor

Choose a reason for hiding this comment

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

discussed in person

@alexeagle
Copy link
Contributor Author

Done, PTAL

Also publish ES2015 snapshots from Travis builds.
@alexeagle alexeagle added the action: merge The PR is ready for merge by the caretaker label Jan 18, 2017
@mhevery mhevery closed this in be6c95a Jan 18, 2017
alexeagle added a commit to alexeagle/angular that referenced this pull request Jan 19, 2017
alxhub pushed a commit that referenced this pull request Jan 19, 2017
tyler-gh pushed a commit to lucidsoftware/angular that referenced this pull request Feb 3, 2017
tyler-gh pushed a commit to lucidsoftware/angular that referenced this pull request Feb 3, 2017
wKoza pushed a commit to wKoza/angular that referenced this pull request Feb 12, 2017
wKoza pushed a commit to wKoza/angular that referenced this pull request Feb 12, 2017
wKoza pushed a commit to wKoza/angular that referenced this pull request Feb 12, 2017
wKoza pushed a commit to wKoza/angular that referenced this pull request Feb 12, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
@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 10, 2019
idlechara pushed a commit to idlechara/angular that referenced this pull request Apr 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants