♻️ Migrate Style and DOM helpers into core/DOM + type-checking #34681
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rcebulko
changed the title
rcebulko
force-pushed
the
core-style
branch
2 times, most recently
from
10606da to
46a6bc4
Compare
|
Hey @erwinmombay! These files were changed: Hey @jridgewell! These files were changed: Hey @alanorozco! These files were changed: Hey @Jiaming-X! These files were changed: Hey @jeffkaufman! These files were changed: Hey @gmajoulet! These files were changed: Hey @processprocess! These files were changed: Hey @t0mg! These files were changed: Hey @mszylkowski! These files were changed: Hey @newmuis! These files were changed: |
rcebulko
force-pushed
the
core-style
branch
3 times, most recently
from
3af1d99 to
27a8bb4
Compare
|
@jridgewell building locally to verify, but I suspect the bundle-size diff is all Brotli noise |
|
Because of how I did the move (temporarily had `main:src/dom.js` vs `pr:src/core/dom/index.js`diff --git a/src/dom.js b/src/core/dom/index.js
index ee39dac4e..f766a3dd3 100644
--- a/src/dom.js
+++ b/src/core/dom/index.js
@@ -14,12 +14,9 @@
* limitations under the License.
*/
-import {Deferred} from './core/data-structures/promise';
-import {dev, devAssert} from './log';
-import {dict} from './core/types/object';
-import {includes} from './core/types/string';
-import {matches} from './core/dom/query';
-import {toWin} from './core/window';
+import {dict} from '../types/object';
+import {matches} from './query';
+import {toWin} from '../window';
const HTML_ESCAPE_CHARS = {
'&': '&',
@@ -31,12 +28,6 @@ const HTML_ESCAPE_CHARS = {
};
const HTML_ESCAPE_REGEX = /(&|<|>|"|'|`)/g;
-/** @const {string} */
-export const UPGRADE_TO_CUSTOMELEMENT_PROMISE = '__AMP_UPG_PRM';
-
-/** @const {string} */
-export const UPGRADE_TO_CUSTOMELEMENT_RESOLVER = '__AMP_UPG_RES';
-
/**
* @typedef {{
* bubbles: (boolean|undefined),
@@ -54,17 +45,15 @@ const DEFAULT_CUSTOM_EVENT_OPTIONS = {bubbles: true, cancelable: true};
* @param {!Element} parent
* @param {function(!Element):boolean} checkFunc
* @param {function()} callback
- * @suppress {suspiciousCode}
+ * @suppress {suspiciousCode} due to IS_ESM
*/
export function waitForChild(parent, checkFunc, callback) {
if (checkFunc(parent)) {
callback();
return;
}
- /** @const {!Window} */
const win = toWin(parent.ownerDocument.defaultView);
if (IS_ESM || win.MutationObserver) {
- /** @const {MutationObserver} */
const observer = new win.MutationObserver(() => {
if (checkFunc(parent)) {
observer.disconnect();
@@ -73,7 +62,6 @@ export function waitForChild(parent, checkFunc, callback) {
});
observer.observe(parent, {childList: true});
} else {
- /** @const {number} */
const interval = win.setInterval(() => {
if (checkFunc(parent)) {
win.clearInterval(interval);
@@ -119,9 +107,7 @@ export function waitForBodyOpenPromise(doc) {
* @param {!Element} element
*/
export function removeElement(element) {
- if (element.parentElement) {
- element.parentElement.removeChild(element);
- }
+ element.parentElement?.removeChild(element);
}
/**
@@ -247,7 +233,7 @@ export function rootNodeFor(node) {
/**
* Determines if value is actually a `ShadowRoot` node.
- * @param {HTMLElement} value
+ * @param {?HTMLElement} value
* @return {boolean}
*/
export function isShadowRoot(value) {
@@ -282,7 +268,7 @@ export function getDataParamsFromAttributes(
const computeParamNameFunc = opt_computeParamNameFunc || ((key) => key);
const {dataset} = element;
const params = dict();
- const paramPattern = opt_paramPattern ? opt_paramPattern : /^param(.+)/;
+ const paramPattern = opt_paramPattern || /^param(.+)/;
for (const key in dataset) {
const matches = key.match(paramPattern);
if (matches) {
@@ -348,37 +334,6 @@ export function iterateCursor(iterable, cb) {
}
}
-/**
- * This method wraps around window's open method. It first tries to execute
- * `open` call with the provided target and if it fails, it retries the call
- * with the `_top` target. This is necessary given that in some embedding
- * scenarios, such as iOS' WKWebView, navigation to `_blank` and other targets
- * is blocked by default.
- *
- * @param {!Window} win
- * @param {string} url
- * @param {string} target
- * @param {string=} opt_features
- * @return {?Window}
- */
-export function openWindowDialog(win, url, target, opt_features) {
- // Try first with the specified target. If we're inside the WKWebView or
- // a similar environments, this method is expected to fail by default for
- // all targets except `_top`.
- let res;
- try {
- res = win.open(url, target, opt_features);
- } catch (e) {
- dev().error('DOM', 'Failed to open url on target: ', target, e);
- }
-
- // Then try with `_top` target.
- if (!res && target != '_top' && !includes(opt_features || '', 'noopener')) {
- res = win.open(url, '_top');
- }
- return res;
-}
-
/**
* Whether the element is a script tag with application/json type.
* @param {!Element} element
@@ -387,8 +342,7 @@ export function openWindowDialog(win, url, target, opt_features) {
export function isJsonScriptTag(element) {
return (
element.tagName == 'SCRIPT' &&
- element.hasAttribute('type') &&
- element.getAttribute('type').toUpperCase() == 'APPLICATION/JSON'
+ element.getAttribute('type')?.toUpperCase() == 'APPLICATION/JSON'
);
}
@@ -400,7 +354,7 @@ export function isJsonScriptTag(element) {
export function isJsonLdScriptTag(element) {
return (
element.tagName == 'SCRIPT' &&
- element.getAttribute('type').toUpperCase() == 'APPLICATION/LD+JSON'
+ element.getAttribute('type')?.toUpperCase() == 'APPLICATION/LD+JSON'
);
}
@@ -459,45 +413,6 @@ export function isIframed(win) {
return win.parent && win.parent != win;
}
-/**
- * Determines if this element is an AMP element
- * @param {!Element} element
- * @return {boolean}
- */
-export function isAmpElement(element) {
- const tag = element.tagName;
- // Use prefix to recognize AMP element. This is necessary because stub
- // may not be attached yet.
- return (
- tag.startsWith('AMP-') &&
- // Some "amp-*" elements are not really AMP elements. :smh:
- !(tag == 'AMP-STICKY-AD-TOP-PADDING' || tag == 'AMP-BODY')
- );
-}
-
-/**
- * Return a promise that resolve when an AMP element upgrade from HTMLElement
- * to CustomElement
- * @param {!HTMLElement} element
- * @return {!Promise<!AmpElement>}
- */
-export function whenUpgradedToCustomElement(element) {
- devAssert(isAmpElement(element), 'element is not AmpElement');
- if (element.createdCallback) {
- // Element already is CustomElement;
- return Promise.resolve(/**@type {!AmpElement} */ (element));
- }
- // If Element is still HTMLElement, wait for it to upgrade to customElement
- // Note: use pure string to avoid obfuscation between versions.
- if (!element[UPGRADE_TO_CUSTOMELEMENT_PROMISE]) {
- const deferred = new Deferred();
- element[UPGRADE_TO_CUSTOMELEMENT_PROMISE] = deferred.promise;
- element[UPGRADE_TO_CUSTOMELEMENT_RESOLVER] = deferred.resolve;
- }
-
- return element[UPGRADE_TO_CUSTOMELEMENT_PROMISE];
-}
-
/**
* Returns true if node is not disabled.
*
@@ -604,7 +519,7 @@ export function dispatchCustomEvent(node, name, opt_data, opt_options) {
const event = node.ownerDocument.createEvent('Event');
// Technically .data is not a property of Event.
- /**@type {?}*/ (event).data = data;
+ event.data = data;
const {bubbles, cancelable} = opt_options || DEFAULT_CUSTOM_EVENT_OPTIONS;
event.initEvent(name, bubbles, cancelable); |
|
@jridgewell bumping this |
|
What is the reason for |
AMPElement doesn't exist yet in core, and it's possible this will be called with Bento elements or something else. PausableInterface is the minimal type contract we can specify here. |
samouri
approved these changes
Jun 9, 2021
jridgewell
approved these changes
Jun 9, 2021
Standardize test name
Fix up types and modernize some syntax
Comment tweaks for dom/fullscreen
Fix stubbed test module
westonruter
added a commit
to westonruter/amphtml
that referenced
this pull request
Jun 11, 2021
…ebook-like-bento-version * 'main' of github.com:ampproject/amphtml: minor updates + fix broken links (ampproject#34840) Add "wrapper": "bento" option to Bento components (ampproject#34838) ♻️ Move src/layout into core to unblock buildDOM for amp-layout (ampproject#34818) ♿ Apply `lang="en"` to relevant snippets in `test/` (ampproject#34768) Bento: Enable `npm` for `amp-video` (ampproject#34822) ✨[story-ads] Introduce new yellow segment progress bar v2 (ampproject#34804) SwG Release (ampproject#34825) 📦 Update build-system devDependencies to v7.14.5 (ampproject#34802) Disable viewport warnings in experiment. (ampproject#34809) Apply lang="en" to examples/ (ampproject#34759) ✨ [Amp story] Scaffold desktop one panel experiment (ampproject#34755) ♻️ Migrate Style and DOM helpers into core/DOM + type-checking (ampproject#34681) 🏗 Don't pull all externs into experiments (ampproject#34800) typechecking: remove pride as not compatible with rest of strategy (ampproject#34787) Fix forbidden terms to unblock `main` (ampproject#34799)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #34096 and #32693
"real" diff: a0f19fb...5e6309c
Moves a bulk of DOM-related modules into core, makes type-checking pass, and modernizes some syntax
Moves the following files:
Resulting structure:
Test files under
test/unit/coremirror this structure