Non-serializable objects in macro config should be a build error

[mydea]

I get this warning message:

Your build is slower because some babel plugins are non-serializable

When I use a regex as macro config. So basically this:

setConfig: {
  '@sentry/ember': {
    blacklistUrls: [
      /graph\.facebook\.com/i,
      /connect\.facebook\.net\/en_US\/all\.js/i,
      // ...
  ],
},

Will trigger the warning. When I replace the array with an array of strings, it works. I can also reproduce it by just using a regex anywhere in the config, like:

setConfig: {
  '@sentry/ember': {
    myField: /myregex/,
  }
}

Or when I pass a built regex (e.g. myfield: new RegExp("regex here")).

[ef4]

Ah, so the bug here is actually that this should have errored and told you that you can only put JSON-serializable things in the macro config. It's probably not really giving you a RegExp in the browser anyway -- it's going to come out the other side as a string.

There's no general purpose way to take an object inside Node and inline it into the browser source code so that it will rehydrate exactly the same.

You can still have regular expressions in your config, but they need to be strings, and then your runtime code would do new RegExp(getOwnConfig().theRegularExpressionString).

That said, I don't think this specific example really needs macros at all, because it's probably not trying to alter the way your code builds. The big reason to use macros is when your addon needs to include different code based on the config. Here you're probably just going to use this list at runtime, with the same implementation regardless of the values. So a fully runtime API would be easier for both you and your users.

For example, @sentry/ember already has an InitSentryForEmber() function that people need to call. This list of URLs could be an argument into that function.

Alternatively, config/environment.js still works the same under Embroider. But it's subject to the same serialization restriction that embroider/macros has, because it's evaluated in Node and the output generates browser code.

[mydea]

Ahhh, thank you, I guess that makes sense. Actually, @sentry/ember already allows to configure this also via the InitSentryForEmber method, I guess it is mostly about actually documenting this and explaining what should be configured where. I made a PR for that: getsentry/sentry-javascript#4426

On the other hand, it would def. make sense to throw an error then when trying to embed a regex or so as a macro, as it basically will silently not work as expected (and is not so clear that it is not allowed when you do not understand how exactly that works behind the scenes). Thank you for the explanation!

[lifeart]

We could have an option here - https://github.com/yahoo/serialize-javascript

[mydea]

Either something like this, or else I can make a PR adding a check like this to the internalSetConfig method:

export default class MacrosConfig {
  private internalSetConfig(fromPath: string, packageName: string | undefined, config: object) {
    if (!isSerializable(config)) {
      throw new Error(
        `[Embroider:MacrosConfig] the given config from '${fromPath}' for packageName '${packageName}' is not JSON serializable.`
      );
    }
   // continue...
}

function isSerializable(obj: object): boolean {
  if (isScalar(obj)) {
    return true;
  }

  if (Array.isArray(obj)) {
    return !obj.some((arrayItem: any) => !isSerializable(arrayItem));
  }

  if (isPlainObject(obj)) {
    for (let property in obj) {
      let value = obj[property] as any;
      if (!isSerializable(value)) {
        return false;
      }
    }

    return true;
  }

  return false;
}

function isScalar(val: any) {
  return (
    typeof val === 'undefined' ||
    typeof val === 'string' ||
    typeof val === 'boolean' ||
    typeof val === 'number' ||
    val === null
  );
}

function isPlainObject(obj: any): obj is Record<string, any> {
  return typeof obj === 'object' && obj.constructor === Object && obj.toString() === '[object Object]';
}