Skip to content

Conversation

@ChristophWurst
Copy link
Member

The marked dependency was out of date and isn't used by any app except
the settings pages. Hence it's move there and updated to resolve security
vulnerabilities.

Replaces #13462

The `marked` dependency was out of date and isn't used by any app except
the settings pages. Hence it's move there and updated to resolve security
vulnerabilities.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@nickvergessen
Copy link
Member

Can we leave it in core? there are quite some apps using it.
Announcements also has an open PR to make use of it, would make it 4 versions of marked being shipped?

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Jan 10, 2019

I assumed it was just settings. That is the issue with this kind of dependency loading. You never know who it uses. And when you update them you don't know which apps break. On that note, there were actually breaking changes in this update, but none effected settings as far as I can tell.

We should aim to get rid of that. Dependencies should be shipped with the apps. This lib is 20kb in size, so the duplication overhead is negligible IMO.

Edit: we're facing a well known problem here: https://en.wikipedia.org/wiki/Dependency_hell. In contrast to php/composer, Javascript will allow us to fix it.

@rullzer
Copy link
Member

rullzer commented Jan 10, 2019

Can we leave it in core? there are quite some apps using it.

Maybe for 1 more version. But apps should ship their own dependencies. Else we can never update a dependency (see jquery) because some app might depends on something

Announcements also has an open PR to make use of it, would make it 4 versions of marked being shipped?

So be it. We do aggressive caching. But else we risk breaking all the apps every time we update a library.

@ChristophWurst
Copy link
Member Author

I'm closing this and will push another PR to bundle a separate version for settings.

@nickvergessen please ship your own for the announcements as well. We'll remove the one from core in Nextcloud 17.

@ChristophWurst ChristophWurst deleted the refactor/move-update-marked-lib branch January 10, 2019 12:41
@nickvergessen
Copy link
Member

Thats a good enough solution. Just dont want to have breaking js apps all the time, just after we spoke about brekaing apps on php level.

@rullzer
Copy link
Member

rullzer commented Jan 10, 2019

@nickvergessen well sure. But this is like using private namespace ;)

@juliusknorr
Copy link
Member

@ChristophWurst @rullzer I've added a note of the deprecation of the shipped marked to #12915. We should keep track there which libraries we want to get rid of in an upcoming version, so developers can prepare 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants