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

classAttr helper: use Object.keys() for iteration? #19

Open
tigt opened this issue Aug 23, 2019 · 2 comments
Open

classAttr helper: use Object.keys() for iteration? #19

tigt opened this issue Aug 23, 2019 · 2 comments

Comments

@tigt
Copy link
Contributor

tigt commented Aug 23, 2019

[email protected]

packages/runtime/src/shared/helpers.js#L16-L20:

for (const name in input) {
  if (input.hasOwnProperty(name) && input[name]) {
    result += ` ${name}`;
  }
}

Some cursory research suggests that not only would Object.keys() be shorter, but it could be faster, too.

What do you think about:

export function classAttr(input) {
  const type = typeof input;

  if (type === "string") {
    return input;
  }

  if (Array.isArray(input)) {
    return input.filter(classAttr).join(' ')
  }
  
  if (type === "object") {
    return Object.keys(input).filter(key => input[key]).join(' ')
  }
}

The styleAttr helper could be rewritten similarly:

export function styleAttr(input) {
  const type = typeof input;

  if (type === "string") {
    return input;
  }

  if (Array.isArray(input)) {
    return input.filter(styleAttr).join(';')
  } 
  
  if (type === "object") {
    return Object.entries(input)
      .filter(([_, val]) => val != null)
      // I’d really rather Marko 5 not do the pixel coercion below, though
      .map(([_, val]) => (typeof val === "number" && val) ? `${val}px` : val)
      .map(([name]) => DASHED_NAMES[name] ||
        (DASHED_NAMES[name] = name.replace(/([A-Z])/g, "-$1").toLowerCase()))
      .map(style => style.join(':'))
      .join(';')
  }
}

…Well, I’m pretty sure the type === "object" codepath is not faster. But it was fun to write, and the Array.isArray codepath looks worth a benchmark.

@tigt
Copy link
Contributor Author

tigt commented Aug 23, 2019

I just realized the result variable is probably because r= is more minified than return , but after dumping both into Terser:

Before (276 bytes):

function classAttrBefore(t){const r=typeof t;let n;if("string"===r)n=t;else{if(n="",Array.isArray(input))for(const t of input){const r=classAttr(t);r&&(n+=` ${r}`)}else if("object"===r)for(const t in input)input.hasOwnProperty(t)&&input[t]&&(n+=` ${t}`);n=n.slice(1)}return n}

After (173 bytes):

function classAttrAfter(t){const r=typeof t;return"string"===r?t:Array.isArray(t)?t.filter(classAttr).join(" "):"object"===r?Object.keys(t).filter(r=>t[r]).join(" "):void 0}

My proposal for styleAttr definitely minifies worse than the original, though.

@tigt
Copy link
Contributor Author

tigt commented Aug 31, 2019

I’ve been benching this for a bit, but since I’m inexperienced at it I can’t really conclude anything…

Other than the following seems definitely faster than the original for…of:

for (const name in Object.keys(input)) {
  if (input[name]) {
    result += ` ${name}`;
  }
}

The .join() operations and such seem to vary based on JIT caching and memory pressure in ways that I can’t explain yet.

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

No branches or pull requests

1 participant