Address review comments

Co-authored-by: Co-authored-by: Timothy Gu <timothygu99@gmail.com>

Author: ExE-Boss

README.md

@@ -4,12 +4,44 @@ A Node JS implementation of the CSS Object Model [CSSStyleDeclaration interface]
 
 [![NpmVersion](https://img.shields.io/npm/v/cssstyle.svg)](https://www.npmjs.com/package/cssstyle) [![Build Status](https://travis-ci.org/jsdom/cssstyle.svg?branch=master)](https://travis-ci.org/jsdom/cssstyle) [![codecov](https://codecov.io/gh/jsdom/cssstyle/branch/master/graph/badge.svg)](https://codecov.io/gh/jsdom/cssstyle)
 
----
+## Background
 
-#### Background
+This package is an extension of the CSSStyleDeclaration class in Nikita Vasilyev's [CSSOM](https://github.com/NV/CSSOM) with added support for CSS 2 & 3 properties. The primary use case is for testing browser code in a Node environment.
 
-This package is an extension of the CSSStyleDeclaration class in Nikita Vasilyev's [CSSOM](https://github.com/NV/CSSOM) with added support for CSS 2 & 3 properties. The primary use case is for testing browser code in a Node environment. 
-
-It was originally created by Chad Walker, it is now maintaind by Jon Sakas and other open source contributors.
+It was originally created by Chad Walker, it is now maintained by Jon Sakas and other open source contributors.
 
 Bug reports and pull requests are welcome.
+
+## APIs
+
+This package exposes two flavors of the `CSSStyleDeclaration` interface depending on the imported module.
+
+### `cssstyle` module
+
+This module default-exports the `CSSStyleDeclaration` interface constructor, with the change that it can be constructed and takes an `onChangeCallback` optional parameter:
+
+```ts
+export = class CSSStyleDeclaration {
+  /**
+   * @param onChangeCallback The callback that is invoked whenever a property changes.
+   */
+  constructor(onChangeCallback?: ((cssText: string) => void) | null);
+}
+```
+
+### `cssstyle/webidl2js-wrapper` module
+
+This module exports the `CSSStyleDeclaration` [interface wrapper API](https://github.com/jsdom/webidl2js#for-interfaces) generated by [webidl2js](https://github.com/jsdom/webidl2js).
+
+#### `privateData`
+
+The `privateData` argument takes the following properties:
+
+```ts
+interface CSSStyleDeclarationPrivateData {
+  /**
+   * The callback that is invoked whenever a property changes.
+   */
+  onChangeCallback?: ((cssText: string) => void) | null
+}
+```

index.js

@@ -6,23 +6,24 @@ const sharedGlobalObject = {};
 webidlWrapper.install(sharedGlobalObject);
 
 const origCSSStyleDeclaration = sharedGlobalObject.CSSStyleDeclaration;
-function CSSStyleDeclaration() {
+
+/**
+ * @constructor
+ * @param {((cssText: string) => void) | null} [onChangeCallback]
+ * The callback that is invoked whenever a property changes.
+ */
+function CSSStyleDeclaration(onChangeCallback = null) {
   if (new.target === undefined) {
     throw new TypeError('Illegal invocation');
   }
 
-  let onChangeCallback = arguments[0];
-  if (onChangeCallback === null || onChangeCallback === undefined) {
-    onChangeCallback = null;
-  } else {
+  if (onChangeCallback !== null) {
     onChangeCallback = webidlConversions.Function(onChangeCallback, {
       context: 'Failed to constructor CSSStyleDeclaration: parameter 1',
     });
   }
 
-  return webidlWrapper.create(sharedGlobalObject, undefined, {
-    onChangeCallback: onChangeCallback,
-  });
+  return webidlWrapper.create(sharedGlobalObject, undefined, { onChangeCallback });
 }
 
 sharedGlobalObject.CSSStyleDeclaration = CSSStyleDeclaration;

lib/CSSStyleDeclaration-impl.js

@@ -21,12 +21,13 @@ class CSSStyleDeclarationImpl {
    * @param {object} privateData
    * @param {((cssText: string) => void) | null} [privateData.onChangeCallback]
    */
-  constructor(globalObject, args, { onChangeCallback = null }) {
+  constructor(globalObject, args, { onChangeCallback }) {
     this._globalObject = globalObject;
-    this._values = {};
-    this._importants = {};
+    this._values = Object.create(null);
+    this._importants = Object.create(null);
     this._length = 0;
     this._onChange = onChangeCallback || (() => {});
+    this.parentRule = null;
   }
 
   /**
@@ -37,24 +38,18 @@ class CSSStyleDeclarationImpl {
    * Returns the empty string if the property has not been set.
    */
   getPropertyValue(name) {
-    if (!this._values.hasOwnProperty(name)) {
-      return '';
-    }
-    return this._values[name].toString();
+    return this._values[name] || '';
   }
 
   /**
    *
    * @param {string} name
    * @param {string} value
-   * @param {string} [priority=null] "important" or null
+   * @param {string} [priority=""] "important" or ""
    * @see http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleDeclaration-setProperty
    */
-  setProperty(name, value, priority) {
-    if (value === undefined) {
-      return;
-    }
-    if (value === null || value === '') {
+  setProperty(name, value, priority = '') {
+    if (value === '') {
       this.removeProperty(name);
       return;
     }
@@ -72,7 +67,14 @@ class CSSStyleDeclarationImpl {
     this._importants[lowercaseName] = priority;
   }
 
-  _setProperty(name, value, priority) {
+  /**
+   * @param {string} name
+   * @param {string | null} value
+   * @param {string} [priority=""]
+   */
+  _setProperty(name, value, priority = '') {
+    // FIXME: A good chunk of the implemented properties call this method
+    // with `value = undefined`, expecting it to do nothing:
     if (value === undefined) {
       return;
     }
@@ -105,7 +107,7 @@ class CSSStyleDeclarationImpl {
    * Returns the empty string if the property has not been set or the property name does not correspond to a known CSS property.
    */
   removeProperty(name) {
-    if (!this._values.hasOwnProperty(name)) {
+    if (!idlUtils.hasOwn(this._values, name)) {
       return '';
     }
 
@@ -164,9 +166,9 @@ Object.defineProperties(CSSStyleDeclarationImpl.prototype, {
         value = this.getPropertyValue(name);
         priority = this.getPropertyPriority(name);
         if (priority !== '') {
-          priority = ' !' + priority;
+          priority = ` !${priority}`;
         }
-        properties.push([name, ': ', value, priority, ';'].join(''));
+        properties.push(`${name}: ${value}${priority};`);
       }
       return properties.join(' ');
     },
@@ -197,13 +199,6 @@ Object.defineProperties(CSSStyleDeclarationImpl.prototype, {
     enumerable: true,
     configurable: true,
   },
-  parentRule: {
-    get: function() {
-      return null;
-    },
-    enumerable: true,
-    configurable: true,
-  },
   length: {
     get: function() {
       return this._length;
@@ -227,6 +222,7 @@ Object.defineProperties(CSSStyleDeclarationImpl.prototype, {
 
 require('./properties')(CSSStyleDeclarationImpl.prototype);
 
+// TODO: Consider using `[Reflect]` for basic properties
 allProperties.forEach(function(property) {
   if (!implementedProperties.has(property)) {
     var declaration = getBasicPropertyDescriptor(property);

scripts/convert-idl.js

@@ -15,16 +15,10 @@ const outputDir = implDir;
 const propertyNames = [
   ...allProperties,
   ...Array.from(allExtraProperties).filter(prop => {
-    return prop !== 'css-float' && !allProperties.has(prop);
+    return !allProperties.has(prop);
   }),
 ].sort();
 
-const camelCasedAttributes = propertyNames.map(n => cssPropertyToIDLAttribute(n));
-const webkitCasedAttributes = propertyNames
-  .filter(n => n.startsWith('-webkit-'))
-  .map(n => cssPropertyToIDLAttribute(n, true));
-const dashedAttributes = propertyNames.filter(n => n.includes('-'));
-
 {
   // TODO: This should be natively supported by WebIDL2JS's Transformer
   // https://github.com/jsdom/webidl2js/issues/188
@@ -43,7 +37,8 @@ partial interface CSSStyleDeclaration {
 `
   );
 
-  for (const camelCasedAttribute of camelCasedAttributes) {
+  for (const property of propertyNames) {
+    const camelCasedAttribute = cssPropertyToIDLAttribute(property);
     genIDL.write(`  [CEReactions] attribute [TreatNullAs=EmptyString] CSSOMString _${camelCasedAttribute};
 `);
   }
@@ -52,7 +47,9 @@ partial interface CSSStyleDeclaration {
   // https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-webkit_cased_attribute
 `);
 
-  for (const webkitCasedAttribute of webkitCasedAttributes) {
+  for (const property of propertyNames) {
+    if (!property.startsWith('-webkit-')) continue;
+    const webkitCasedAttribute = cssPropertyToIDLAttribute(property, /* lowercaseFirst = */ true);
     genIDL.write(`  [CEReactions] attribute [TreatNullAs=EmptyString] CSSOMString _${webkitCasedAttribute};
 `);
   }
@@ -61,8 +58,9 @@ partial interface CSSStyleDeclaration {
   // https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-dashed_attribute
 `);
 
-  for (const dashedAttribute of dashedAttributes) {
-    genIDL.write(`  [CEReactions] attribute [TreatNullAs=EmptyString] CSSOMString ${dashedAttribute};
+  for (const property of propertyNames) {
+    if (!property.includes('-')) continue;
+    genIDL.write(`  [CEReactions] attribute [TreatNullAs=EmptyString] CSSOMString ${property};
 `);
   }
 
@@ -78,7 +76,7 @@ partial interface CSSStyleDeclaration {
 
 const transformer = new Transformer({
   implSuffix: '-impl',
-  // TODO: Add support for `CEReactions`
+  // TODO: Add support for `[CEReactions]`
 });
 
 transformer.addSource(srcDir, implDir);

scripts/generate_implemented_properties.js

@@ -6,11 +6,15 @@ const t = require('babel-types');
 const generate = require('babel-generator').default;
 const { idlAttributeToCSSProperty } = require('../lib/parsers');
 
+const webkitPropertyName = /^webkit[A-Z]/;
 const dashedProperties = fs
   .readdirSync(path.resolve(__dirname, '../lib/properties'))
-  .filter(propertyFile => propertyFile.slice(-3) === '.js')
+  .filter(propertyFile => path.extname(propertyFile) === '.js')
   .map(propertyFile => {
-    return idlAttributeToCSSProperty(propertyFile.slice(0, -3), propertyFile.startsWith('webkit'));
+    return idlAttributeToCSSProperty(
+      path.basename(propertyFile, '.js'),
+      webkitPropertyName.test(propertyFile)
+    );
   });
 
 const out_file = fs.createWriteStream(path.resolve(__dirname, '../lib/implementedProperties.js'), {