DEV Community

Discussion on: Render Browser Specific Content with React 🎉

View post
Collapse
 
mbtts profile image
mbtts • • Edited

Thank you for sharing.

Did you consider using Browserslist and if so what was the reason for rejecting it?

A small couple of points on the the code.

The final else if is redundant and can be replaced with else. All of the tests will still pass because it is impossible to fail to satisfy this condition once reached.

  if (all) {
  } else if (none) {
  } else if (except) {
  } else if (only) {
  } else if (!all && !only && !except && !none) {
  }

  if (all) {
  } else if (none) {
  } else if (except) {
  } else if (only) {
  } else {
  }

As you can see the final two blocks contain identical logic:

  } else if (only) {
    allBrowsers.forEach((browser) => {
      if (props[browser]) allowedBrowsers.push(browser);
      else restrictedBrowsers.push(browser);
    });
  } else {
    allBrowsers.forEach((browser) => {
      if (props[browser]) allowedBrowsers.push(browser);
      else restrictedBrowsers.push(browser);
    });
  }

So this can be simplified further to just:

  if (all) {
  } else if (none) {
  } else if (except) {
  } else {
  }

I think it can be simplified further as the logic is the same for the final two blocks - they just swap the two list variables (allowedBrowsers/restrictedBrowsers) around. I am not sure it is worth extracting it to a function though - it is a trade off between explicitness and duplication.

  } else if (except) {
    allBrowsers.forEach(browser => {
      if (props[browser]) restrictedBrowsers.push(browser);
      else allowedBrowsers.push(browser);
    });
  } else {
    allBrowsers.forEach(browser => {
      if (props[browser]) allowedBrowsers.push(browser);
      else restrictedBrowsers.push(browser);
    });
  }
Collapse
 
mbtts profile image
mbtts • • Edited

I've had a bit longer to think about the shouldRenderForBrowser function and below are some suggestions:

  1. Defer determining the current browser until it is required (i.e. if the all or none props are provided the current browser is irrelevant).
  2. It is not necessary to maintain two lists (restricted and allowed) because by definition any browser which is restricted cannot be allowed and vice versa.
  3. The number of return statements can be reduced as false is the default.

This leads to something like this:

const shouldRenderForBrowser = (props, browsers) => {
  const { except, all, none } = props;

  if (all) {
    return true;
  }

  if (!none) {
    const currentBrowser = browsers.find(browser => browser.isCurrentBrowser);

    const browserProps = browserCheck.allBrowsers.filter(
      browser => props[browser]
    );

    const browserMatches =
      currentBrowser && browserProps.includes(currentBrowser.name);

    return except ? !browserMatches : browserMatches;
  }

  return false;
};
Collapse
 
flexdinesh profile image
Dinesh Pandiyan •

This is brilliant.

I ported this code from an old project which was built in a hurry. As I ported, I failed to look into optimization points thinking it should already be optimized. This is what happens when you trust your code blindly.

Thanks for all these pointers. I'm gonna refactor the component again and loop in all these suggestions. You are amazing!

Thread Thread
 
flexdinesh profile image
Dinesh Pandiyan •

Also, if you're interested in creating a PR, feel free to play around.