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

Don't use nested objects to "namespace" namespace constants #21073

Merged
merged 1 commit into from
Mar 24, 2021
Merged

Don't use nested objects to "namespace" namespace constants #21073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/react-dom/src/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ import {
setValueForStyles,
validateShorthandPropertyCollisionInDev,
} from '../shared/CSSPropertyOperations';
import {Namespaces, getIntrinsicNamespace} from '../shared/DOMNamespaces';
import {HTML_NAMESPACE, getIntrinsicNamespace} from '../shared/DOMNamespaces';
import {
getPropertyInfo,
shouldIgnoreAttribute,
Expand Down Expand Up @@ -86,8 +86,6 @@ const CHILDREN = 'children';
const STYLE = 'style';
const HTML = '__html';

const {html: HTML_NAMESPACE} = Namespaces;

let warnedUnknownTags;
let suppressHydrationWarning;

Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/client/setInnerHTML.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @flow
*/

import {Namespaces} from '../shared/DOMNamespaces';
import {SVG_NAMESPACE} from '../shared/DOMNamespaces';
import createMicrosoftUnsafeLocalFunction from '../shared/createMicrosoftUnsafeLocalFunction';
import {enableTrustedTypesIntegration} from 'shared/ReactFeatureFlags';

Expand All @@ -25,7 +25,7 @@ const setInnerHTML = createMicrosoftUnsafeLocalFunction(function(
node: Element,
html: {valueOf(): {toString(): string, ...}, ...},
): void {
if (node.namespaceURI === Namespaces.svg) {
if (node.namespaceURI === SVG_NAMESPACE) {
if (__DEV__) {
if (enableTrustedTypesIntegration) {
// TODO: reconsider the text of this warning and when it should show
Expand Down
8 changes: 4 additions & 4 deletions packages/react-dom/src/server/ReactPartialRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ import {
setCurrentPartialRenderer,
} from './ReactPartialRendererHooks';
import {
Namespaces,
HTML_NAMESPACE,
getIntrinsicNamespace,
getChildNamespace,
} from '../shared/DOMNamespaces';
Expand Down Expand Up @@ -747,7 +747,7 @@ class ReactDOMServerRenderer {
type: null,
// Assume all trees start in the HTML namespace (not totally true, but
// this is what we did historically)
domNamespace: Namespaces.html,
domNamespace: HTML_NAMESPACE,
children: flatChildren,
childIndex: 0,
context: emptyObject,
Expand Down Expand Up @@ -1327,12 +1327,12 @@ class ReactDOMServerRenderer {
const tag = element.type.toLowerCase();

let namespace = parentNamespace;
if (parentNamespace === Namespaces.html) {
if (parentNamespace === HTML_NAMESPACE) {
namespace = getIntrinsicNamespace(tag);
}

if (__DEV__) {
if (namespace === Namespaces.html) {
if (namespace === HTML_NAMESPACE) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this is really weird. We track the namespace in prod and the only thing we use it for is this warning.

We don't need the namespace on the server since it's implied by the HTML.

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like it was intentional. #10452

But I'm just going to remove this because it's not worth the overhead/complexity when we don't bother with this on the client anyway.

// Should this check be gated by parent namespace? Not sure we want to
// allow <SVG> or <mATH>.
if (tag !== element.type) {
Expand Down
12 changes: 3 additions & 9 deletions packages/react-dom/src/shared/DOMNamespaces.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,9 @@
* LICENSE file in the root directory of this source tree.
*/

const HTML_NAMESPACE = 'http://www.w3.org/1999/xhtml';
const MATH_NAMESPACE = 'http://www.w3.org/1998/Math/MathML';
const SVG_NAMESPACE = 'http://www.w3.org/2000/svg';

export const Namespaces = {
html: HTML_NAMESPACE,
mathml: MATH_NAMESPACE,
svg: SVG_NAMESPACE,
};
export const HTML_NAMESPACE = 'http://www.w3.org/1999/xhtml';
export const MATH_NAMESPACE = 'http://www.w3.org/1998/Math/MathML';
export const SVG_NAMESPACE = 'http://www.w3.org/2000/svg';

// Assumes there is no parent namespace.
export function getIntrinsicNamespace(type: string): string {
Expand Down