-
Notifications
You must be signed in to change notification settings - Fork 205
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
feat: warn on element redefinitions, point to resolution docs #3136
Conversation
@Westbrook what do you think re: this approach? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the idea of leveraging the Dev Mode Messging System to support educating users about this situation. It makes we think we should add a build
type and move this message to the highest
level. Pairing it with content around possible fixes/workarounds in the link will be great.
I really DON'T like that this prevents the error. Without the error, it's hard to beleive any of our consumers does anything about this issue. Particularly, the team I've see (anecdotally) encounter this situation the most is specifically developing in a way that they should get this error until they actually get their architecture right.
Structurally, for Dev Mode messages, the if (window.__swc?.DEBUG) {
must be the outter most condition so that it can tree shake the largest amount of the work being done when built for production.
I agree with not suprressing the error, you only way you get folks to reliably fix things is to put roadblocks in their path to doing it the 'wrong' way. Would welcome this dev message though, can't land soon enough in my opinion! :-) |
b8b6ad2
to
df3350d
Compare
@Westbrook @benjamind updated to still fail, but to inject a dev mode warning before failure Also added a testing helper so we can be sure this is actually working in each component. I've converted all the "A" components so far. |
Tachometer resultsChromeaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
asset permalink
avatar permalink
badge permalink
banner permalink
button-group permalink
button permalink
card permalink
checkbox permalink
coachmark permalink
color-area permalink
color-handle permalink
color-loupe permalink
color-slider permalink
color-wheel permalink
dialog permalink
divider permalink
dropzone permalink
field-group permalink
field-label permalink
grid permalink
help-text permalink
icon permalink
icons permalink
illustrated-message permalink
link permalink
menu permalink
meter permalink
number-field permalink
overlay permalink
picker-button permalink
picker permalink
popover permalink
progress-bar permalink
progress-circle permalink
quick-actions permalink
radio permalink
search permalink
sidenav permalink
slider permalink
split-button permalink
split-view permalink
swatch permalink
switch permalink
table permalink
tabs permalink
tags permalink
textfield permalink
thumbnail permalink
toast permalink
tooltip permalink
top-nav permalink
tray permalink
underlay permalink
Firefoxaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
asset permalink
avatar permalink
badge permalink
banner permalink
button-group permalink
button permalink
card permalink
checkbox permalink
coachmark permalink
color-area permalink
color-handle permalink
color-loupe permalink
color-slider permalink
color-wheel permalink
dialog permalink
divider permalink
dropzone permalink
field-group permalink
field-label permalink
grid permalink
help-text permalink
icon permalink
icons permalink
illustrated-message permalink
link permalink
menu permalink
meter permalink
number-field permalink
overlay permalink
picker-button permalink
picker permalink
popover permalink
progress-bar permalink
progress-circle permalink
quick-actions permalink
radio permalink
search permalink
sidenav permalink
slider permalink
split-button permalink
split-view permalink
swatch permalink
switch permalink
table permalink
tabs permalink
tags permalink
textfield permalink
thumbnail permalink
toast permalink
tooltip permalink
top-nav permalink
tray permalink
underlay permalink
|
ae57006
to
73f79ab
Compare
274be9b
to
da8bce9
Compare
tools/base/src/define-element.ts
Outdated
window.__swc.warn( | ||
undefined, | ||
`Attempted to redefine <${name}>. This usually indicates that multiple versions of the same web component were loaded onto a single page.`, | ||
'https://TODO' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the actual documentation for this coming along?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First draft: https://dev-mode-define-element--spectrum-web-components.netlify.app/registry-conflicts/
Thoughts on what should go into yarn & pnpm?
Closing to retain work history, shifting PR to #3171 |
Description
This is a working example of one path towards addressing versioning challenges for consumers.
Currently, SWC packages that are imported twice (either pre-built and included on the same page, or via different dependency versions in a single build) will fail at runtime with a custom element registration error.
This change converts that runtime error to a development-time warning, with the ability to direct consumers to documentation about resolving transitive dependency versions etc.
It also demonstrates how we can verify correct behavior in tests, via an example in sp-button (the tests are currently passing, but failing code coverage requirements since this adds a few lines of currently-uncovered code to each package).
The changes are a quick grep, so there may be extra packages to cover in a similar manner. Icons?
Related issue(s)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main
.