Skip to content

refactor(docs-infra): simplify custom-element polyfill setup - #25806

Closed
robwormald wants to merge 2 commits into
angular:masterfrom
robwormald:aio-polyfills-update
Closed

refactor(docs-infra): simplify custom-element polyfill setup#25806
robwormald wants to merge 2 commits into
angular:masterfrom
robwormald:aio-polyfills-update

Conversation

@robwormald

@robwormald robwormald commented Sep 4, 2018

Copy link
Copy Markdown
Contributor

Simplify the Custom Element polyfills for AIO. Previously, we had to conditionally load either the polyfill (for browsers without CE support) or the es5-shim (for browsers with native CE support to enable ES5 "classes"). The polyfills now are simplified, and shouldn't require conditional loading anymore, so we can shift them to the polyfills.ts file to simplify things.

Assuming this approach is successful, we'll follow this up with a similar change to the @angular/elements schematics setup

@mary-poppins

mary-poppins commented Sep 4, 2018

Copy link
Copy Markdown

You can preview cc610f2 at https://pr25806-cc610f2.ngbuilds.io/.

JoostK
JoostK reviewed Sep 4, 2018
View reviewed changes
Comment thread aio/yarn.lock Outdated

@JoostK JoostK Sep 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you include package.json as well?

@robwormald robwormald Sep 4, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@robwormald
robwormald requested a review from IgorMinar September 4, 2018 19:15
@robwormald robwormald added the action: merge The PR is ready for merge by the caretaker label Sep 4, 2018
@ngbot

ngbot Bot commented Sep 4, 2018

Copy link
Copy Markdown

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required label: "PR target: *"
    failure missing required status "code-review/pullapprove"
    pending status "ci/circleci: build-packages-dist" is pending
    pending status "ci/circleci: aio_preview" is pending
    pending status "ci/circleci: test_ivy_aot" is pending
    pending status "ci/circleci: test" is pending
    pending status "ci/circleci: lint" is pending
    pending status "ci/circleci: test_ivy_jit" is pending
    pending status "continuous-integration/travis-ci/pr" is pending
    pending 1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mary-poppins

mary-poppins commented Sep 4, 2018

Copy link
Copy Markdown

You can preview 8ba72c8 at https://pr25806-8ba72c8.ngbuilds.io/.

@mary-poppins

mary-poppins commented Sep 4, 2018

Copy link
Copy Markdown

You can preview c717818 at https://pr25806-c717818.ngbuilds.io/.

@IgorMinar IgorMinar added the target: patch This PR is targeted for the next patch release label Sep 4, 2018
IgorMinar
IgorMinar approved these changes Sep 4, 2018
View reviewed changes

@IgorMinar IgorMinar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@robwormald the next time please include the size increase change in the original change so that the two are connected in history. thanks

gkalpak
gkalpak reviewed Sep 5, 2018
View reviewed changes
Comment thread aio/src/polyfills.ts
import 'zone.js/dist/zone'; // Included with Angular CLI.

import '@webcomponents/custom-elements'; // Custom Elements Polyfill
import '@webcomponents/custom-elements/src/native-shim';

@gkalpak gkalpak Sep 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Won't this result in unnecessarily loading more code in both borwsers that don't support Custom Elements and those that do?

@robwormald robwormald Sep 5, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the shim is very minimal, and it's a lot less gross than the document.write trick we're doing now. We'll revisit this when we look into shipping modern (esm/es2015) builds from CLI.

@gkalpak gkalpak Sep 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For reference, the polyfill (which is what I was more worried about) is 15K (minified), so nothing to worry about atm (with our 476K main bundle 😁).

mhevery pushed a commit that referenced this pull request Sep 5, 2018
@robwormald @mhevery
refactor(docs-infra): simplify custom-element polyfill setup (#25806)
46de203 
PR Close #25806
@mhevery mhevery closed this in 59aab14 Sep 5, 2018
mhevery pushed a commit that referenced this pull request Sep 5, 2018
@robwormald @mhevery
refactor(docs-infra): bump polyfills payload limit (#25806)
25c1f33 
PR Close #25806
mhevery pushed a commit that referenced this pull request Sep 5, 2018
@robwormald @mhevery
refactor(docs-infra): bump polyfills payload limit (#25806)
4596fc0 
PR Close #25806
gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 5, 2018
@gkalpak
build(docs-infra): restore correct dependency versions in package.json
5146b15 
Accidentally broken in 46de203 (while cherry-picking angular#25806 into
6.1.x).
@gkalpak gkalpak mentioned this pull request Sep 5, 2018
Merged
mhevery pushed a commit that referenced this pull request Sep 6, 2018
@gkalpak @mhevery
build(docs-infra): restore correct dependency versions in `package.js…
d1063c6 
…on` (#25827)

Accidentally broken in 46de203 (while cherry-picking #25806 into
6.1.x).
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 14, 2019

Copy link
Copy Markdown

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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JoostK JoostK JoostK left review comments
+2 more reviewers
@gkalpak gkalpak gkalpak left review comments
@IgorMinar IgorMinar IgorMinar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@robwormald @mary-poppins @JoostK @IgorMinar @gkalpak @googlebot