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.

Sign up for GitHub

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

fix: Don't import * from focus-trap to avoid default export confusion #4485

Merged
merged 7 commits into from
Mar 11, 2019
Merged

fix: Don't import * from focus-trap to avoid default export confusion #4485

merged 7 commits into from
Mar 11, 2019

Conversation

kfranqueiro
Copy link
Contributor

Fixes #4484.

abhiomkar
abhiomkar approved these changes Mar 8, 2019
View reviewed changes
Copy link
Contributor

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

LGTM, added a comment to replace this in one more place.

packages/mdc-drawer/component.ts Outdated Show resolved Hide resolved
abhiomkar
abhiomkar reviewed Mar 8, 2019
View reviewed changes
Copy link
Contributor

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

nit: fix: => chore: when you merge it.

acdvorak
acdvorak previously requested changes Mar 8, 2019
View reviewed changes
Copy link
Contributor

@acdvorak acdvorak left a comment

Choose a reason for hiding this comment

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

Any idea why unit and screenshot tests are failing on dialog?

packages/mdc-dialog/util.ts Outdated Show resolved Hide resolved
@acdvorak
Copy link
Contributor

acdvorak commented Mar 8, 2019

I think fix is correct because this fixes bug #4484

@kfranqueiro
Copy link
Contributor Author

I think the screenshot tests are diffing because of the same reason the unit tests are failing. The interesting part is that this error sounds eerily similar to the error that #4484 is experiencing in 1.0.0...

@codecov-io
Copy link

codecov-io commented Mar 11, 2019 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (master@0dc25f4). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #4485   +/-   ##
========================================
  Coverage          ?     99%           
========================================
  Files             ?     130           
  Lines             ?    6232           
  Branches          ?     807           
========================================
  Hits              ?    6170           
  Misses            ?      61           
  Partials          ?       1
Impacted Files Coverage Δ
packages/mdc-dialog/component.ts 100% <ø> (ø)
packages/mdc-dialog/util.ts 100% <100%> (ø)
packages/mdc-drawer/util.ts 100% <100%> (ø)
packages/mdc-drawer/component.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dc25f4...7dad2ec. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 627 screenshot tests passed for commit 3a3e741 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 627 screenshot tests passed for commit 7a8a303 vs. master! 💯🎉

@kfranqueiro
Copy link
Contributor Author

So a few things happened to lead to where this branch is at now.

  • The absence of esModuleInterop is what caused this to show up in some environments but not others... and then invert which cases passed/failed when I attempted to fix it.
  • Adding esModuleInterop caused marked increases in the size of our UMD bundles when combined with importHelpers, because basically every module now ends up requiring the helpers, and webpack pulls in the full tslib import whereas tsc only emits the necessary functions.
  • importHelpers is desirable for individual ES modules, because otherwise you end up with various subsets of tslib emitted in every individual module.
  • TL;DR: this PR turns off importHelpers in the tsconfig to avoid UMD bundle bloat, but adds it via CLI for build:esmodules specifically.

@mdc-web-bot
Copy link
Collaborator

All 627 screenshot tests passed for commit 0dc013a vs. master! 💯🎉

@kfranqueiro kfranqueiro dismissed acdvorak’s stale review March 11, 2019 21:11

Issues have been resolved

@mdc-web-bot
Copy link
Collaborator

All 627 screenshot tests passed for commit 65cf42a vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 627 screenshot tests passed for commit 7dad2ec vs. master! 💯🎉

@kfranqueiro kfranqueiro merged commit 6082dc3 into master Mar 11, 2019
@kfranqueiro kfranqueiro deleted the fix/focus-trap-imports branch March 11, 2019 21:41
@moog16 moog16 mentioned this pull request Mar 14, 2019
Closed
@gertcuykens
Copy link

gertcuykens commented Mar 15, 2019 •
edited

#4507

https://jsbin.com/saqoyinufi/edit?html,console,output

hmm could it be something is still not right?

@kfranqueiro kfranqueiro mentioned this pull request Mar 20, 2019
Closed
adrianschmidt pushed a commit to Lundalogik/material-components-web that referenced this pull request Mar 29, 2019
@kfranqueiro
fix: Don't import * from focus-trap to avoid default export confusion (…
bd3d946 
…material-components#4485)

(cherry picked from commit 6082dc3)
adrianschmidt pushed a commit to Lundalogik/material-components-web that referenced this pull request Mar 29, 2019
@kfranqueiro
fix: Don't import * from focus-trap to avoid default export confusion (…
65fd9d9 
…material-components#4485)

(cherry picked from commit 6082dc3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhiomkar abhiomkar abhiomkar approved these changes

@moog16 moog16 moog16 approved these changes

@acdvorak acdvorak acdvorak left review comments

Assignees

@acdvorak acdvorak

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@kfranqueiro @acdvorak @codecov-io @mdc-web-bot @gertcuykens @abhiomkar @moog16 @googlebot