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

chore(build): remove the es6 output #4454

Closed
wants to merge 1 commit into from

Conversation

alexeagle
Copy link
Contributor

We don't rely on it for anything, so we can speed up the build and simplify

@alexeagle alexeagle added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 1, 2015
We don't rely on it for anything, so we can speed up the build and simplify
@IgorMinar
Copy link
Contributor

lgtm, but you need to rebase it against master again.

@IgorMinar IgorMinar added pr_state: LGTM action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 2, 2015
@IgorMinar IgorMinar assigned alexeagle and unassigned IgorMinar Oct 2, 2015
@pkozlowski-opensource
Copy link
Member

@alexeagle thnx for this PR, I saw that you've done some changes that I was planning to do as well (dropping traceur related transpilation). I've sent a follow-up PR #4502 to clean up some more stuff.

@IgorMinar
Copy link
Contributor

@alexeagle what happened with this PR? can we get it in? :)

@alexeagle
Copy link
Contributor Author

I am stuck on the cyclical dependency problem. I've spent quite a bit of
time trying a few approaches:

  • drop the cycle check. But Marc says we still need it, because while CJS
    has some loader shim to make it possible, it can introduce quirky behavior
    that has cost us debugging time in the past
  • check for cycles against the TS sources instead of the es6 emit. But
    Madge doesn't work against our TS sources, I've spent a couple hours
    tracking down the cause but haven't gotten it yet
  • check for cycles against the es5 emit, but it's broken because we
    actually do have cycles in the module dependency graph, which do not exist
    in the symbol dependency graph. one fix would be
    • fix the module-level cycles. There are two, and one was fixable, but
      the other is hard and Tobias said it should just stay that way.

On Mon, Nov 2, 2015 at 10:29 AM Igor Minar notifications@github.com wrote:

@alexeagle https://github.com/alexeagle what happened with this PR? can
we get it in? :)


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

@naomiblack naomiblack modified the milestone: beta-00 Nov 2, 2015
@IgorMinar
Copy link
Contributor

@alexeagle how is circular-dep check related to this PR? we currently run it on the es6 output anyway.

also, have you seen #5022?

@alexeagle
Copy link
Contributor Author

It's related because if we remove the es6 output, we have to check for
circular dependencies on some other tree.
I had not seen that bug, but those are the same circular deps I observed. I
suspect you are seeing the same thing I described: these are
module-circular-deps but not symbol-circular-deps.

On Mon, Nov 2, 2015 at 10:34 PM Igor Minar notifications@github.com wrote:

@alexeagle https://github.com/alexeagle how is circular-dep check
related to this PR? we currently run it on the es6 output anyway.

also, have you seen #5022 #5022
?


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

@mgol
Copy link
Member

mgol commented Nov 4, 2015

Just one remark - many projects don't need to support IE or the Android Browser and such projects in a year may very well work only in ES6-capable browsers without transpilation (Edge already has almost 90% score in the kangax test so this is not a distant future). A pure ES6 build would be useful for such purposes.

People can always compile Angular themselves from TypeScript to ES6 although that's harder.

@alexeagle alexeagle mentioned this pull request Nov 16, 2015
@vsavkin
Copy link
Contributor

vsavkin commented Nov 16, 2015

Closing as we decided to keep ES6 output for now.

@vsavkin vsavkin closed this Nov 16, 2015
@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.
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants