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

fix: drop support for Edge 18 and Safari 13 #3648

Merged
merged 5 commits into from
Aug 8, 2023

Conversation

nolanlawson
Copy link
Contributor

@nolanlawson nolanlawson commented Aug 1, 2023

Details

Drops support for Edge 18 and Safari 13. Removes (almost) all the last vestiges of compat mode from the codebase.

Context on browser support

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Technically this is not a breaking change, because we already dropped official support for these browsers in LWC v3.0.0.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

GUS work item

W-13818697

@nolanlawson nolanlawson requested a review from a team as a code owner August 1, 2023 22:55
@nolanlawson
Copy link
Contributor Author

/nucleus test

@@ -46,7 +46,7 @@ commands:
coverage:
type: boolean
default: false
compat:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided the word compat was too overloaded at this point, so I renamed this to legacy_browsers.

@@ -15,7 +15,6 @@ type E = HostElement;
export type LifecycleCallback = (elm: E) => void;

export interface RendererAPI {
isNativeShadowDefined: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we are dropping Edge 18, all of our tested browsers support shadow DOM.

@@ -5,7 +5,6 @@
"scripts": {
"start": "karma start ./scripts/karma-configs/test/local.js",
"test": "karma start ./scripts/karma-configs/test/local.js --single-run --browsers ChromeHeadless",
"test:compat": "COMPAT=1 yarn test",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was unused anyway.

COMPAT,
DISABLE_SYNTHETIC: !SYNTHETIC_SHADOW_ENABLED,
},
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dunno what this was doing. According to SauceLabs, it is "User-defined custom data that will accept any valid JSON object, limited to 64KB in size." I assume it just goes in the Circle CI UI somewhere.

@@ -44,7 +43,6 @@ function createEnvFile() {
window.process = {
env: {
NODE_ENV: ${JSON.stringify(NODE_ENV_FOR_TEST || 'development')},
COMPAT: ${COMPAT},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our tests no longer need to know if they are running in a compat browser!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't be fooled – this file is just renamed from create-custom-element-using-upgradable-constructor and GitHub doesn't realize it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out I forgot to remove this unused file a while back, in #3310

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're probably gonna want to use Hide whitespace in the GitHub UI for all these test files.

Copy link
Contributor

@jye-sf jye-sf left a comment

Choose a reason for hiding this comment

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

A couple minor nits, but otherwise LGTM. It's happening!

}

export const isNativeShadowRootDefined = !isNull(NativeShadowRoot);
const NativeShadowRoot = ShadowRoot;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we still need NativeShadowRoot or can that be removed and just have isInstanceOfNativeShadowRoot reference ShadowRoot directly? It's only referenced twice in this file and not exported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we still need to capture the globalThis.ShadowRoot once, since synthetic shadow will override it. But I can definitely clean up this logic: 766bfab

}
export { assignedNodes, assignedElements };
export const assignedNodes = HTMLSlotElement.prototype.assignedNodes;
export const assignedElements = HTMLSlotElement.prototype.assignedElements;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how many of these types of assignments could be refactored away in the synthetic-shadow package now that more and more of them are just extra layers of assignments with no added wrapper functionality. (e.g. window.ts).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sure a lot of it could, yeah. I held back from heavy refactoring, though, since synthetic shadow is deprecated, and minifiers will clean up a lot of these abstraction layers anyway.

@nolanlawson
Copy link
Contributor Author

/nucleus test

@nolanlawson nolanlawson merged commit a0bc2a6 into master Aug 8, 2023
11 checks passed
@nolanlawson nolanlawson deleted the nolan/drop-old-edge-safari branch August 8, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants