[Bug Fix] Omit "Warning: Received false for a non-boolean attribute crossOrigin."

[mwskwong]

No description provided.

[NoriSte]

Thanks, I'll release it in the next few days!

[NoriSte]

What about replacing the if that comes before this couple of lines and the lines themselves with a switch statement?
Something like

switch(domainOption.crossOrigin) {
  case undefined:
    return { domain, crossOrigin: '' }

  case false:
    // Returning `undefined` in case of a falsy value prevents the following warning
    // > Warning: Received `false` for a non-boolean attribute `crossOrigin`.
    return { domain, crossOrigin: undefined }

  case '':
    return { domain, crossOrigin: '' }

  case 'anonymous':
    return { domain, crossOrigin: 'anonymous' }

  case 'use-credentials':
    return { domain, crossOrigin: 'use-credentials' }

  default:
    throw new Error(
      `gatsby-plugin-preconnect: cannot parse \`crossOrigin\` from ${domainOption.crossOrigin}. Expected \`undefined\`, \`\`, \`false\`, \`true\`, \`anonymous\`, or \`use-credentials\`.`
    )
}

(if I made a mistake with the cases please let me know).

by doing so:

• we reduce the cognitive load that following the evolution of crossOrigin requires
• we don't need to use a mutable variable

what do you think?

[mwskwong]

Yep, I like this approach more actually. I decided to make the changes the way it was is because I want to change a few things as possible.

BTW, I feel that corssOrigin should be default undefined since you are supposed to use it only when you actually need to, not the other way around.

So ideally it would look something like this:

If crossOrigin = undefined --> don't specify in HTML
Otherwise --> Match values accoding to the spec

This is a breaking change though.

https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/crossorigin

[NoriSte]

I was thinking the same, the rationales should be

• if the key exists AND it's undefined -> don't specify in HTML
• if the key exists -> specify it

Why? To manage the first two cases reportedin the README

domains: [
        'https://foo.com', // <-- THIS
        'https://bar.com', // <-- THIS
        { domain: 'https://enablecors.com', crossOrigin: true },
        { domain: 'https://disablecors.com', crossOrigin: false },
        { domain: 'https://corswithanonymous.com', crossOrigin: 'anonymous' },
        { domain: 'https://corswithcreds.com', crossOrigin: 'use-credentials' },
      ],

[mwskwong]

If that's the case. Should I forget about this pull request and make another one to make this happen? It also solves the original problem as well, since function-wise

{ domain: 'https://disablecors.com', crossOrigin: false }

is eqivilant to

{ domain: 'https://disablecors.com' }

[NoriSte]

My bad, I wanted to write

• if the key exists AND it's undefined -> don't specify in HTML
• if the key DOES NOT exists -> specify it

I'm sorry! Why do you prefer to raise a new PR?

[mwskwong]

My bad, I wanted to write

• if the key exists AND it's undefined -> don't specify in HTML
• if the key DOES NOT exists -> specify it

I'm sorry! Why do you prefer to raise a new PR?

So let me make sure we are on the same page. Are we agree to the following logic?

const validCrossOriginValues = [
  "anonymous",
  "use-credentials",
  "",
  true,
  undefined // the default value
];

export const parseOptions = (domainOptions) => {
  return domainOptions.map((domainOption) => {
    const domain = typeof domainOption === 'string' ? domainOption : domainOption.domain
    if (!domain) {
      throw new Error(
        `gatsby-plugin-preconnect: cannot parse \`domain\` from ${domainOption}. Expected a string or \`{domain: string}\``
      )
    }

    const crossOriginValid = validCrossOriginValues.some(value => value === domainOption.crossOrigin);
    if (!crossOriginValid)
      throw new Error(`gatsby-plugin-preconnect: cannot parse \`crossOrigin\` from ${domainOption.crossOrigin}. Expected one of ${validCrossOrigin}`);

    return { domain, crossOrigin }
  })
}

[NoriSte]

Yep, but with a "more readable" case, like

export const parseOptions = (domainOptions) => {
  return domainOptions.map((domainOption) => {
    // ... add comments...
    if (!domainOption || !domainOption.domain) {
      throw new Error(
        `gatsby-plugin-preconnect: cannot parse \`domain\` from ${domainOption}. Expected a string or \`{domain: string}\``
      )
    }

    // ... add comments...
    if(typeof domainOption === 'string') {
      return { domain, crossOrigin: '' }
    }

    switch(domainOption.crossOrigin) {
      // ... explain the rational behind `undefined`
      case undefined:
      case '':
      case true:
        return { domain, crossOrigin: '' }
      
      case false:
        // Returning `undefined` in case of a falsy value prevents the following warning
        // > Warning: Received `false` for a non-boolean attribute `crossOrigin`.
        return { domain, crossOrigin: undefined }
      
      case 'anonymous':
        return { domain, crossOrigin: 'anonymous' }
      
      case 'use-credentials':
        return { domain, crossOrigin: 'use-credentials' }
    }

    throw new Error(
      `gatsby-plugin-preconnect: cannot parse \`crossOrigin\` from ${domainOption.crossOrigin}. Expected \`undefined\`, \`\`, \`false\`, \`true\`, \`anonymous\`, or \`use-credentials\`.`
    )
  })
}

my purpose is getting straightforward what the code returns for every option, allowing the future developer less jumping between conditions and ternaries to understand the returned value, what do you think?

Proposal

More: what do you think about introducing one more option: disabled? Through this, a dynamic configuration can leave the configuration objects as is but control them through the disable key. Something like

domains: [
  { domain: 'https://enablecors.com', crossOrigin: true, disabled: true|false },
  { domain: 'https://disablecors.com', crossOrigin: false, disabled: true|false },
  { domain: 'https://corswithanonymous.com', crossOrigin: 'anonymous', disabled: true|false },
   { domain: 'https://corswithcreds.com', crossOrigin: 'use-credentials', disabled: true|false },
 ],

the disabled key is optional, obviously. But the enabled option would be the subject for another PR, sorry for messing up this PR, let's discuss it in a dedicated issue.

[NoriSte]

@mwskwong are you going to follow up on this? No hurry at all, just to know if I had to go on all my own starting from your suggestions and changes or you will work on in as soon as you have time 😊