Remove LoDash dependency

[ChrisMBarr]

My Angular project uses ChordsheetJS and it gave this this warning upon build about Chordsheet using LoDash, which is not an ESM module

Looking into it, I saw ChordsheetJS only used LoDash's _.get() method, and it only ever used it to access a single property. Seems like an easy one to just access directly with a helper method that has a default/fallback of an empty string

[ChrisMBarr]

Not sure what the deal is with that failure, it seems unrelated to any of the changes I made since all the tests pass

[martijnversluis]

Hey @ChrisMBarr. Thanks so much for taking the time to look at this! I do agree it would be great to drop all runtime dependencies.

Unfortunately, being able to get any key from metadata is critical to support the chordpro standard. For example, there are official metadata directives and you can define and use arbitrary values (like {x_myapp_some_setting}). It is also possible to get array values. See also the docs for metadata.get.

This means, if we remove the dependency, that we have to cover the existing functionality.

If you would like help with that, surely let me know! 🙏

[ChrisMBarr]

Oh ok, I didn't realize that get was a public method! Ok, I have something I can fall back to using something like this: https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_get

[martijnversluis]

Ok, I have something I can fall back to using something like this: https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_get

That sure looks like a great replacement!

[martijnversluis]

@ChrisMBarr Would you like to implement that? Please let me know might you need help 👍

[ChrisMBarr]

Ok, this should be good to go now. Unit tests pass, however I did notice that there are no unit tests to verify how .get() itself works, to be able to access arbitrary properties or access array items. It should work, but it might be a good idea to add those just in case.

[martijnversluis]

Thanks for your work! 🙏

Yes, apparently unit tests are missing, I would've expected those would fail when removing the lodash package.

Would you like me to add those tests?

[ChrisMBarr]

I started to look into this... but from what I can tell the metadata.get method as documented here uses a completely different method than the one I re-wrote, and that one already has tests written for it

Is this one that previously used LoDash even public at all? Does it even need any tests? It appear that the only thing that uses this are the internal calls to configuration.get('metadata.separator') which could just be re-written as configuration.metadata.separator to directly access what is needed instead of doing a weird string path thing.

[martijnversluis]

I started to look into this... but from what I can tell the metadata.get method as documented here uses a completely different method than the one I re-wrote, and that one already has tests written for it

You're completely right. I really thought Lodash was used in metadata, not in configuration 😅

Is this one that previously used LoDash even public at all? Does it even need any tests? It appear that the only thing that uses this are the internal calls to configuration.get('metadata.separator') which could just be re-written as configuration.metadata.separator to directly access what is needed instead of doing a weird string path thing.

It think you're right that the usages can be rewritten. The class could use some tests, but that was also the case with Lodash. I'm fine with configuration.metadata.separator 👍

[ChrisMBarr]

Alright, that's now in so there is no more configuration.get method at all! There's just a getter method/property that will always return a string. Much simpler now, and one less dependency

[martijnversluis]

Great job! I would suggest one rename, other than that this is good to go! 👍

[codeclimate]

Code Climate has analyzed commit f99d87c and detected 0 issues on this pull request.

View more on Code Climate.

[martijnversluis]

Awesome @ChrisMBarr! Thanks for contributing 🎉