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

Convert blocking-elements to TypeScript #16

Merged
merged 32 commits into from
Jul 29, 2019
Merged

Convert blocking-elements to TypeScript #16

merged 32 commits into from
Jul 29, 2019

Conversation

aomarks
Copy link
Contributor

@aomarks aomarks commented Jul 25, 2019

  • Convert to TypeScript. NOTE: I strongly recommend reading the "Convert to TypeScript" commit (cd01c88) separately from the rest.

  • [BREAKING] Remove WebComponents V0 support

  • [BREAKING] /dist/blocking-elements.js is now ES2017 instead of ES5 (i.e. it now has classes). /dist/blocking-elements.min.js is still ES5. /blocking-elements.js doesn't exist anymore.

  • [BREAKING] Remove HTML import file.

  • Clang-format, and replace eslint with tslint.

  • dist/ files are no longer checked in, instead we ensure npm run build is run in prepack.

  • Add full text LICENSE file.

Copy link
Collaborator

@valdrinkoshi valdrinkoshi left a comment

Choose a reason for hiding this comment

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

Great update! Nits/questions around the IntertableHTMLElement interface, the current version forces runtime checks that we could probably avoid.

destructor() {
// Restore original inertness.
this[_restoreInertedSiblings](this[_topElParents]);
const nullable = this as unknown as {
Copy link
Collaborator

Choose a reason for hiding this comment

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

these properties can be updated to accept null, and you wouldn't need this cast here, e.g.

private[_blockingElements]: IntertableHTMLElement[]|null = [];

But I guess you did it here because otherwise you'd need to add checks everywhere for them being defined, right? If so, mind adding a comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly. Done.

/**
* Adds the element to the blocking elements.
*/
push(element: HTMLElement): HTMLElement|undefined {
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix the return type to void

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// if it's inertable and if already inert.
if (this[_isInertable](oldInert) && !oldInert.inert) {
oldInert.inert = true;
if (siblingsToRestore) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check here and in L214 is new, and will show in the runtime too.
This condition should never happen (siblingsToRestore undefined), because we call this method on elements that passed through _inertSiblings method which sets this variable on the element.
I think we should update all IntertableHTMLElement properties to be non-optional, and remove these checks here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I minimized the amount of casting we need by splitting into HasInternalState and a MaybeHasInternalState, and only casting for _topElParents, which was the only place we needed it.

private[_restoreInertedSiblings](elements: IntertableHTMLElement[]) {
elements.forEach((el) => {
const mo = el[_parentMO];
if (mo !== undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above, these checks will show at runtime. We should probably change IntertableHTMLElement properties to be non-optional, and remove these checks here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, see other comment.

for (const element of elements) {
const children =
element.parentNode !== null ? element.parentNode.children : [];
const inertedSiblings = new Set<HTMLElement>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

type should be <IntertableHTMLElement>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, see other comment.

element[_parentMO] =
new MutationObserver(this[_handleMutations].bind(this));
const mo = element[_parentMO];
if (element.parentNode !== null && mo !== undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

avoid these checks:

const mo = new MutationObserver(this[_handleMutations].bind(this));
element[_parentMO] = mo;
mo.observe(element.parentNode!, {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

this.pop();
return;
}
if (inertedSiblings && inertedSiblings.has(sibling)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

check for inertedSiblings not needed if we make _siblingsToRestore non-optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, see other comment.

toKeepInert.add(sibling);
} else {
sibling.inert = true;
if (inertedSiblings) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

check not needed if we make _siblingsToRestore non-optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, see other comment.

node_modules
bower_components
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

package.json Outdated
@@ -2,26 +2,28 @@
"name": "blocking-elements",
"version": "0.0.2",
"description": "A polyfill for the proposed blocking elments stack API",
"main": "dist/blocking-elements.js",
"main": "blocking-elements.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be .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.

Oops, this shouldn't actually have changed.

rollem.config.js Outdated
],
}, {
entry: 'blocking-elements.js',
entry: 'dist/blocking-elements.js',
Copy link
Collaborator

Choose a reason for hiding this comment

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

The generated js does not have an IIFEE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

As discussed in chat, /dist/blocking-elements.js is now an ES module that exports a singleton instead of setting the document.$blockingElements global. The .min.js file does still set the global, and continues to be IIFE-guarded ES5 minified. Included usage instructions in the README.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Uhm, I'm wondering if we should rather create a new dist/blocking-elements-module.js for the ES module case. The README.md is centered around the document.$blockingElements API, so probably dist/blocking-elements.js and dist/blocking-elements.min.js should differ only on minification (both should be IFEEd and set the document.$blockingElements global).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, it is a little weird. Both files now set document.$blockingElement.

@@ -0,0 +1,407 @@
/**
* @license
Copy link
Collaborator

Choose a reason for hiding this comment

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

license currently not included in the dist/* files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. For some reason the rollup license-preserving text wasn't working. I upgraded to the latest rollup and babel, removed rollem (not needed anymore), and also tweaked the script to only emit each unique license once (since there are now two input files).

@aomarks
Copy link
Contributor Author

aomarks commented Jul 26, 2019

PTAL! Note I moved the source files (of which there are now two) into src/, sorry for the churn.

@@ -25,65 +25,6 @@
}
}

describe('ShadowDom v0', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👋 🎉

package.json Outdated
"build": "rollem"
"format": "clang-format --style=file -i src/*.ts",
"prepack": "npm run build",
"build": "rm dist/* -f && tsc && rollup -c && rm dist/document-global.*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: should be rm -rf dist/ && ... here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I wanted to rm dist/* instead of rm dist on on purpose because in the past I've been bitten by tools that are watching the directory's inode getting confused by the directory going away. Rewrote a little though.

rollem.config.js Outdated
],
}, {
entry: 'blocking-elements.js',
entry: 'dist/blocking-elements.js',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Uhm, I'm wondering if we should rather create a new dist/blocking-elements-module.js for the ES module case. The README.md is centered around the document.$blockingElements API, so probably dist/blocking-elements.js and dist/blocking-elements.min.js should differ only on minification (both should be IFEEd and set the document.$blockingElements global).

inert?: boolean;
}

interface InternalState {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Throwing this here: the BlockingElements class could as well keep track of all this via couple WeakMaps, but that would be quite some refactoring (which probably does belong to a separate PR)

// Top changed only if the removed element was the top element.
if (i === this[_blockingElements].length) {
const top = this.top;
if (top !== null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this check is not needed, _topChanged allows for top === null:

this[_topChanged](this.top);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

mo.disconnect();
(element as MaybeHasInternalState)[_parentMO] = undefined;
const siblings = element[_siblingsToRestore];
if (siblings !== undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can remove this check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

A ShadyDOM root acts like a real ShadowRoot, but isn't actually in the
real DOM tree, so it's not observable. Observe its host instead, which
is the real DOM parent of the shady children.
@aomarks
Copy link
Contributor Author

aomarks commented Jul 27, 2019

PTAL!

This includes a fix for an exception that was being thrown in the tests when ShadyDOM was enabled. I'm not 100% sure about it, but it does make the tests pass.

In investigating that issue, I also found a case that crashes with native shadow roots, described at #18. I included a test case for it in this PR (currently marked as .skip).

Also, I have not been able to get Safari to report success from Sauce, even though from the Sauce videos, Safari is passing 100% of tests. I assume this is some issue with Safari/sauce/easy-sauce. For now I have removed Safari and filed #17 to investigate later. I also bumped the version of Edge we're testing from 14 to "latest".

Copy link
Collaborator

@valdrinkoshi valdrinkoshi left a comment

Choose a reason for hiding this comment

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

This LGTM 👍 Nice find about #18, thanks for filing it and adding a test. There are some typos in the test itself, anyways I think #19 should fix both the issue and enable the test.

@aomarks aomarks merged commit 54e853d into master Jul 29, 2019
@aomarks aomarks deleted the typescript branch July 29, 2019 18:20
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.

2 participants