multiformats / js-cid

CID implementation in JavaScript
MIT License
97 stars 39 forks source link

Build issue in Angular 8 project (with resolution) #96

Closed qu37zal closed 4 years ago

qu37zal commented 4 years ago

I have an Angular project using some ipfs related packages with cids as a dependency. My ng build fails with the following error:

ERROR in ./node_modules/cids/src/index.js Module not found: Error: Can't resolve 'multicodec/src/base-table'

Upon investigation, I found that cids has multicodec as a dependency, and attempts to import a json file using require without specifying the file suffix. This results in the file not being identified and the aforementioned error. By specifying 'multicodec/src/base-table.json' the issue can be bypassed.

I'm happy to submit a patch if you like. I'd like to get this fixed in the upstream so I don't have to patch it locally in a postinstall step. I believe it is not considered best practice to import json in this way, so I'd propose it be imported using fs.readFile/JSON.parse, but the remedy above works as well.

vmx commented 4 years ago

@mikeal What do you think about removing CID.codecs completely?

mikeal commented 4 years ago

It’s a breaking change so we shouldn’t consider that the fix for this change. I think we should experiment with something like require(‘cids/bare’) first before we go and make a big breaking change. For now let’s just take the easy .json fix.

vmx commented 4 years ago

I just think that having codecs isn't a good idea anyway, so let's just do that breaking change.

leandroyalet commented 4 years ago

Same issue on a project using Webpack, I fixed it by adding an alias on webpack.config file:

        resolve: {
            alias: {
                'multicodec/src/base-table': path.dirname(
                    require.resolve('multicodec/src/base-table.json')
                )
            }
        },
rvagg commented 4 years ago

let's just get this specific issue fixed before making more radical changes, adding explicit .json extensions in require is good practice anyway #97

ilirhushi commented 4 years ago

Hi I created a PR to fix this issue: https://github.com/multiformats/js-cid/pull/100

vmx commented 4 years ago

@qu37zal @leandroyalet @ilirhushi I've just released version 0.7.2. Could you please confirm that it fixes this issue?

leandroyalet commented 4 years ago

@vmx yes, works correctly now

ilirhushi commented 4 years ago

@vmx yep for me too, thanks 🚀