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

getSourceDirectory() in utils/frost-icon-svg.js does not account for the use of npm link #583

Closed
6 of 10 tasks
notmessenger opened this issue Jul 9, 2018 · 0 comments
Closed
6 of 10 tasks
Labels
bug Copied Copied to frost-core (JIRA)

Comments

@notmessenger
Copy link
Contributor

notmessenger commented Jul 9, 2018

BUG REPORT

  • I have ensured that the issue isn't already reported
  • I have confirmed that the issue is reproducible with the latest released version
  • I have deleted the FEATURE REQUEST / CODE CHANGE section

Summary

The resolution of this issue may address #568 and #578

The testing instructions (being) introduced in #582 need to be updated to account for the additional npm link test case.

Expected Behavior

When using npm link to develop an addon that provides an icon pack, expect getSourceDirectory() in utils/frost-icon-svg.js to return the correct value.

Actual Behavior

An error is thrown about not being able to find whichever addon has been npm linked from <module_path>/node_modules/ember-frost-core/utils.

Possible Solution

Do not use npm link

The real solution though is to improve getSourceDirectory() so that it supports the use of npm link.

@sophypal has provided this information from when he encountered this issue:

Here’s a result from debugging the call in `resolve.sync`

> basedir
'/frost/alarms-common/node_modules/ember-frost-core/utils'
> loadNodeModulesSync(x, '/frost/mcp-ui')
'/frost/mcp-ui/node_modules/alarms-common/index.js'
> loadNodeModulesSync(x, '/frost/alarms-common')
undefined
> loadNodeModulesSync(x, '/frost/alarms-common/node_modules/ember-frost-core/utils')
undefined

`require.resolve` seems to work better.

> require.resolve(this.moduleName())
'/frost/alarms-common/index.js'
> this.moduleName()
'alarms-common'

`resolve.sync` with no `basedir` set defaults to `dirname` of the caller. If `ember-frost-core` is found at the root of the dep tree, it’s `'/frost/mcp-ui/node_modules/ember-frost-core/utils'` and it works because now the path includes the app which has the resolution. This explains why this works.

The reason why it didn’t work in my case was because the addon resolved the `ember-frost-core` to a path that included the symlink, and not the path you might expect in a typical install. So this does seem isolated to me doing things like `npm link` and `npm install ../` which symlinks the file path.

Also, `require.resolve(this.moduleName() + '/frost-icon-svgs')` seems to work in the npm link case

Should be able to replace https://github.com/ciena-frost/ember-frost-core/blob/master/utils/frost-icon-svg.js#L85-L86 with the `require.resolve` line. `require.resolve` seems to be smarter about where npm is actually getting the module from.

resolve.sync was initially used because it is the direction Ember Cli has headed but it needs to be researched whether it is specifically recommended to be used over require.resolve.

Steps to Reproduce

  1. npm link an addon which provides an icon pack
  2. Run either ember build or ember serve

Environment

(answer all that are applicable)

  • I am using the latest released version (can check with npm ls <package-name>)
  • I am using these browsers:
    • Latest Chrome
    • Latest Firefox
  • My version of Node is: 8.6.0
  • My version of npm is: 5.3.0
  • My OS is: macOS High Sierra 10.13.5 (17F77)
  • Include the contents of your package.json file
@dafortin dafortin added the Copied Copied to frost-core (JIRA) label Sep 6, 2018
@dafortin dafortin closed this as completed Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Copied Copied to frost-core (JIRA)
Projects
None yet
Development

No branches or pull requests

2 participants