rwjblue / ember-cli-cjs-transform

MIT License
48 stars 12 forks source link

Transformation addon does not respect `resolveFrom` option passed to `app.import` #72

Open amiller-gh opened 6 years ago

amiller-gh commented 6 years ago

Problem

Tested in ember-cli 2.18.2 and 3.3.0

The AMD transform allows users to specify an optional resolveFrom. This allows addons to call app.import and reference their dependencies instead of resolving all node modules relative to the app.

// In file `/path/to/app/node_modules/addon-name/index.js`

// Resolves to `/path/to/app/node_modules/path/to/thing.js` 
app.import("node_modules/path/to/thing.js",{
  using: [{ transformation: 'cjs', as: 'asset-name' }],
});

// Resolves to `/path/to/app/node_modules/addon-name/node_modules/path/to/thing.js` 
app.import("node_modules/path/to/thing.js",{
  using: [{ transformation: 'cjs', as: 'asset-name' }],
  resolveFrom: __dirname,
});

The CJS transform attempts to resolve the file on disk here and here. However, the resolveFrom option is never passed – it always tries to resolve from parent.root.

This results in an error when the CJS transform can't find the module.

Potential Fixes

It doesn't appear that ember-cli passes the resolveFrom option to the custom transforms. If we want to enable this use case without changes to ember-cli, the option will need to be passed twice in userland:

// Duplicate options, but minimal changes and easier rollout
app.import("node_modules/path/to/thing.js",{
  using: [{ transformation: 'cjs', as: 'asset-name', resolveFrom: __dirname }],
  resolveFrom: __dirname,
});

Otherwise, the resolveFrom option can be passed along to custom transforms' processOptions hook to be made available for use: https://github.com/ember-cli/ember-cli/blob/v3.0.x/lib/broccoli/ember-app.js#L1711

Let me know if I'm off base here! Happy to help make these changes if you'd like to move forward with a fix.

rwjblue commented 6 years ago

In general, addons should avoid app.import and should instead use this.import which should automatically resolve dependencies relative to the addon itself.

I dont know that I have tests for that though, definitely happy to confirm/deny. Also, it still probably does make sense to support resolveFrom just to stay consistent...

rwjblue commented 6 years ago

https://github.com/rwjblue/ember-cli-cjs-transform/blob/1c04d6654b968cdeb4e567c4a704f7cea9e33eee/tests/transform-test.js#L292-L294

This test seems related?

amiller-gh commented 6 years ago

Could've sworn that I tried this.import as well – I thought the same thing seeing it try to resolve from parent.root, but parent always seemed to be the app, not the addon. Will confirm 👍