fix: lookup svg-core using require.resolve

[knownasilya]

Package managers move modules around all the time, and one of those moves is module hoisting, where it's moved up to a parent where a module can be share across multiple packages. See related issue: microsoft/rushstack#1998

[knownasilya]

Test failure looks unrelated to my change.

[jrjohnson]

Thanks @knownasilya, I don't remember the specifics, but it looks like I added this in #92 to deal with yarn workspaces specifically. I don't use workspaces in any project so I'll need to create a test environment to validate this in. The resolution of this directory here might become less important depending on the results of #149 as well.

[jrjohnson]

@knownasilya I can confirm that this will break yarn workspaces where node_modules isn't in the current working directory. I have no idea how to resolve this conflict. Maybe @mlwilkerson has some idea about what the Rollup process is doing and how we can accommodate both of these scenarios.

If anyone wants to try out the workspace I created https://github.com/jrjohnson/fontawesome-test-workspace to run it do
yarn install in the top level
cd sweet-ass-app
yarn start

[knownasilya]

Would love to see a fix for this.

[jrjohnson]

Me too, but I don't have any ideas that account for all build tools. I'm actually hoping that template imports can help with this. You can currently pass an icon object into the component instead of an icon string, if it becomes easier to import that icon object from NPM into the template then that will probably become the preferred API here. Combining this with embroider splitting it would handle auto-shaking the icon tree as well.

[knownasilya]

Could it be a config based setting? At least in the intermediate state.

[jrjohnson]

I'm all for it, but I just don't know enough about how different package managers might resolve this. Gonna ping @mlwilkerson again from the Fontawesome team to see if he has any more experience with the many package management options and resolving the library.

[knownasilya]

Any movement here? Far as I know this is still an issue and I'm still using this patch.

[dknutsen]

bump, i'm running into this now and using this patch atm

[jrjohnson]

I can't merge this PR as is because it for sure breaks yarn workspaces (which we now support for good or ill, but it would be breaking to stop supporting them). I'm happy to merge a config change instead, I just don't quite know how to make that work. Idea and expertise from someone more knowledgable about how node does package lookup super welcome!

The bigger issue on the horizon is embroider which doesn't really support this type of build time manipulation, but does support rollup directly (which we use here). I'm also pondering if it would be better to require the icon to be imported directly instead of referenced by a string. This would be especially supported by the template imports that are in Ember's near future.

All that is to say that I'd love to fix this, but I just don't know how to do it for al use cases and package managers in a future proof way that doesn't require too much churn. Maybe the best thing at this point would be to allow you to turn off the build and find a way to handle this in your application directly?

[dknutsen]

@jrjohnson makes sense. I will probably try to use yarn workspaces so definitely would like compatibility with that.

Icon imports seem like a logical step, especially since that sorta solves the static analysis problem.

Anyway thanks for the update!

[knownasilya]

Maybe something like https://github.com/ncochard/webpack-path-resolve/blob/master/packages/webpack-path-resolve/src/index.ts could help (package name is misleading, since it doesn't have any specific reason it should be tied to webpack)? But I'm unsure.

[navels]

Hit this error when we switched from yarn to pnpm, this patch resolves it.