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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

robwormald
Copy link
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
Copy link

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

JoostK
JoostK reviewed Sep 4, 2018
View reviewed changes
aio/yarn.lock Outdated
@@ -469,8 +469,8 @@
long "^3.2.0"

"@webcomponents/custom-elements@^1.0.8":
version "1.0.8"
resolved "https://registry.yarnpkg.com/@webcomponents/custom-elements/-/custom-elements-1.0.8.tgz#b7b8ef7248f7681d1ad4286a0ada5fe3c2bc7228"
version "1.2.0"
Copy link
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?

Copy link
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 added the action: merge The PR is ready for merge by the caretaker label Sep 4, 2018
@ngbot
Copy link

ngbot bot commented Sep 4, 2018

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
Copy link

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

@mary-poppins
Copy link

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
Copy link
Contributor

@IgorMinar IgorMinar left a comment

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
aio/src/polyfills.ts
@@ -36,7 +36,8 @@
* Zone JS is required by Angular itself.
*/
import 'zone.js/dist/zone'; // Included with Angular CLI.

import '@webcomponents/custom-elements'; // Custom Elements Polyfill
import '@webcomponents/custom-elements/src/native-shim';
Copy link
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?

Copy link
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.

Copy link
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).
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
refactor(docs-infra): simplify custom-element polyfill setup (angular…
6647914 
…#25806)

PR Close angular#25806
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
refactor(docs-infra): bump polyfills payload limit (angular#25806)
01a759e 
PR Close angular#25806
@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 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

@gkalpak gkalpak gkalpak left review comments

@IgorMinar IgorMinar IgorMinar approved these changes

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.

None yet
6 participants
@robwormald @mary-poppins @JoostK @IgorMinar @gkalpak @googlebot