Skip to content
This repository has been archived by the owner on Jun 15, 2021. It is now read-only.

lodash dependecy still exists. #495

Closed
code-smith opened this issue Jul 26, 2016 · 10 comments · Fixed by #506
Closed

lodash dependecy still exists. #495

code-smith opened this issue Jul 26, 2016 · 10 comments · Fixed by #506

Comments

@code-smith
Copy link
Contributor

The latest scribe-editor installed through npm results in dependency errors.
It says cannot resolve module 'lodash-amd/modern/string/escape' in the below file
https://github.com/guardian/scribe/blob/master/src/plugins/core/formatters/plain-text/escape-html-characters.js

lodash-amd is not listed as dependency,so relevant files are not downloaded.
I had to add [email protected] as dependency to resolve this,the latest lodash-amd doesn't maintain same folder structure, so it's of no help.

This error doesn't not happen when scribe is downloaded from bower. The reason being there are polyfills for that particular dependency thats being added to main scribe.js source file.

I propose adding same polyfill to the file mentioned above to resolve this issue instead of adding and entire obsolete library.
Will make an PR and reference this issue.
Thanks

@brandondurham
Copy link

Thank you for including the temp fix here, @code-smith .

@code-smith
Copy link
Contributor Author

code-smith commented Jul 28, 2016

Can someone merge the PR and release it? It would be of great help !
@OliverJAsh

@shaundillon
Copy link
Contributor

@code-smith There's a PR open to include [email protected] in package.json this will fix this. While I agree using the whole library may be a bit much, this dependency is used by many of scribe's plugins anyway.

@code-smith
Copy link
Contributor Author

code-smith commented Jan 30, 2017

@ShaunYearStrong This is not the right way to go ahead.
let me give some reasons not to do so.
#1 Lots of code changes in previous releases were aimed at making library independent of lodash, this will just undo everything.
#2 [email protected] is older version, most use lodash-4 , its very different in code structure to 3.5. Any user application will end up having both lodash-4 and lodash-3.5 which can be avoided if the polyfill is accepted

It's your decision, but personally i don't think its the right way to go ahead.
Thanks anyways.

@shaundillon shaundillon reopened this Jan 31, 2017
@shaundillon
Copy link
Contributor

@code-smith - Good point, I didn't notice the move away from lodash dependencies. In the plugin libraries, there was an effort to normalise the version of lodash used which is why I made this decision.

@code-smith
Copy link
Contributor Author

@ShaunYearStrong thanks for reconsidering your decision. Also note that lodash-amd 4.0 has a very different code structure; most use the latest version , so it will just add another library to user's application code base which is not desirable.
Merging the polyfill PR is a good solution if you ask me

@shaundillon
Copy link
Contributor

shaundillon commented Jan 31, 2017

@code-smith - It would work, I'm not a fan of including another library's code in here though.
I think a good solution would be to import the single escape method, available here: https://www.npmjs.com/package/lodash.escape

EDIT: Just realised that's not AMD

@code-smith
Copy link
Contributor Author

@ShaunYearStrong The ployfill i included is an optimal solution that would fix everything.

@shaundillon
Copy link
Contributor

@code-smith I've spoken to some colleagues and they agree it's the best way. I'll merge it now but release will be someone else, as we're running through quite a few issues atm. Thanks for your work!

@code-smith
Copy link
Contributor Author

@ShaunYearStrong thanks and really appreciate the gesture. I used the editor to build an in-house cms and i must say this is the best out there. You guys rock !!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants