-
Notifications
You must be signed in to change notification settings - Fork 49
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
fix: lookup svg-core using require.resolve #153
base: master
Are you sure you want to change the base?
Conversation
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
Test failure looks unrelated to my change. |
even if main is changed this will give use a consistent path
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. |
@knownasilya I can confirm that this will break yarn workspaces where If anyone wants to try out the workspace I created https://github.com/jrjohnson/fontawesome-test-workspace to run it do |
Would love to see a fix for this. |
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. |
Could it be a config based setting? At least in the intermediate state. |
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. |
Any movement here? Far as I know this is still an issue and I'm still using this patch. |
bump, i'm running into this now and using this patch atm |
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? |
@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! |
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. |
Hit this error when we switched from |
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