guardian / scribe

DEPRECATED: A rich text editor framework for the web platform

http://guardian.github.io/scribe/

Apache License 2.0

3.51k stars 245 forks source link

lodash dependecy still exists. #495

Closed code-smith closed 7 years ago

code-smith commented 7 years ago

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 lodash-amd@3.5.0 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 commented 7 years ago

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

code-smith commented 7 years ago

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

shaundillon commented 7 years ago

@code-smith There's a PR open to include lodash-amd@3.5.0 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 commented 7 years ago

@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 lodash-amd@3.5.0 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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

shaundillon commented 7 years ago

@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 commented 7 years ago

@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 !!