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

Prevent foot-guns when customizing index.js #9334

Closed
NullVoxPopuli opened this issue Sep 17, 2020 · 8 comments
Closed

Prevent foot-guns when customizing index.js #9334

NullVoxPopuli opened this issue Sep 17, 2020 · 8 comments

Comments

@NullVoxPopuli
Copy link
Contributor

For example, let's say you want to add

options: {
  autoImport {
    // some config
  }
}

it seems that if another addon specifies those options, whichever is loaded last wins.

so, it'd be great if ember-cli could either warn/hard-error when there is a conflict in options or, forbid the setting of options for other addons within your addon (I recognize that a deep merge of all options wouldn't be predictable, because there isn't a good way to guarantee order).

If this is already supported, then there are other foot guns I've yet to track down :(

@rwjblue
Copy link
Member

rwjblue commented Sep 17, 2020

I don't understand the issue, let's focus on the actual problem that you faced (not the general case of attempting to prevent all footguns). What did you have specified in your index.js? What didn't work?

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Sep 17, 2020

the full index.js was:

module.exports = {
  name: require('./package').name;
  options: {
    // but i'm not sure if specifics matter
    autoImport {
       ['splunk-sdk']: 'splunk-sdk/client/splunk',
    },
    skipBabel: [
      {
        package: 'splunk-sdk',
        semverRange: '*',
      }
    ]
  }
}

but in both the dummy app and all consuming apps the autoImport config acted as if it wasn't there. (error thrown when trying to import a specific module, splunk-sdk)

what did work was the exact same autoImport config, but in ember-cli-build.js.

Knowing that, to get things working, I deleted the options object, and used the included hook to find the root project and install / merge the autoImport config on that.

@NullVoxPopuli
Copy link
Contributor Author

Code used for included hook:

  included() {
    let app = this._findRootApp();

    let autoImport = app.options.autoImport || {};

    app.options.autoImport = {
      ...autoImport,
      alias: {
        ...(autoImport.alias || {}),
        ['splunk-sdk']: 'splunk-sdk/client/splunk',
      },
      skipBabel: [
        ...(autoImport.skipBabel || []),
        { package: 'splunk-sdk', semverRange: '*' }
      ]
   };

    this._super.included.apply(this, arguments);
  },

  _findRootApp() {
    let current = this;

    while (current.parent.parent) {
      current = current.parent;
    }

    return current.app;
  },

@rwjblue
Copy link
Member

rwjblue commented Sep 17, 2020

You basically can't use options like that as far as I can tell (since the addon model clobbers this.options with its own thing).

this.__originalOptions = this.options = defaultsDeep(this.options, {
babel: this[BUILD_BABEL_OPTIONS_FOR_PREPROCESSORS](),
});

I would expect you need to do:

module.exports = {
  name: require('./package').name,

  init() {
    this._super.init.apply(this, arguments);

    this.options.autoImport = {
      alias: {
        'splunk-sdk': 'splunk-sdk/client/splunk',
      },
      skipBabel: [
        { package: 'splunk-sdk', semverRange: '*' }
      ],
    };
  }
}

Can you confirm that works properly?

@chrismllr
Copy link

@rwjblue Ah, so in the above, would the apps options then be deep-merged on top of the this.options.autoImport above? This looks nice for instance where the host app's options don't need to be referenced to decide what to add in.

@NullVoxPopuli
Copy link
Contributor Author

@rwjblue why can't I use options? I just successfully uses options in addons to set ember-cli-babel to enable typescript transpiling. Is there a difference between ember-cli-babel and auto-import options? or are not all options treated the same?

@locks
Copy link
Contributor

locks commented Aug 9, 2023

@NullVoxPopuli issue still relevant/warranted?

@NullVoxPopuli
Copy link
Contributor Author

Nope, none of these problems exist with v2 addons

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

4 participants