NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
10 Mar 2022

Pull Request for Issue # .

Kinda replaces #36906 (no FA upgrade to v6)

Summary of Changes

  • patch the font awesome files
  • upgrade to choices v10
  • local fixes

Testing Instructions

  • Apply this Pr's branch
  • run npm ci
  • Check that nothing is broken visually and there are no console deprecation when running any node (css related) cli command

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

@brianteeman @bembelimen

avatar dgrammatiko dgrammatiko - open - 10 Mar 2022
avatar dgrammatiko dgrammatiko - change - 10 Mar 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 10 Mar 2022
Category JavaScript Repository NPM Change
e37f93f 10 Mar 2022 avatar dgrammatiko CS
avatar dgrammatiko dgrammatiko - change - 10 Mar 2022
Labels Added: NPM Resource Changed ?
avatar joomla-cms-bot joomla-cms-bot - change - 10 Mar 2022
Category JavaScript Repository NPM Change Administration com_media NPM Change JavaScript Repository
5362f4a 10 Mar 2022 avatar dgrammatiko meh
49d2787 10 Mar 2022 avatar dgrammatiko grrr
avatar dgrammatiko dgrammatiko - change - 10 Mar 2022
Title
[4.1] Remove the math.div deprecation
[4.1][nocache] Remove the math.div deprecation
avatar dgrammatiko dgrammatiko - edited - 10 Mar 2022
avatar dgrammatiko dgrammatiko - change - 10 Mar 2022
Title
[4.1][nocache] Remove the math.div deprecation
[4.1][NO CACHE] Remove the math.div deprecation
avatar dgrammatiko dgrammatiko - edited - 10 Mar 2022
3c8a789 10 Mar 2022 avatar dgrammatiko order
bec8d8f 10 Mar 2022 avatar dgrammatiko fo
avatar bembelimen
bembelimen - comment - 11 Mar 2022

I guess the vue file is wrong?

avatar dgrammatiko
dgrammatiko - comment - 11 Mar 2022

No, they are correct. It’s the result of npm run lint:js — —fix

avatar HLeithner
HLeithner - comment - 12 Mar 2022

The silent upgrade of choices.js is a b/c break

avatar dgrammatiko
dgrammatiko - comment - 12 Mar 2022

The silent upgrade of choices.js is a b/c break

it wasn't silent, check the description. Also fixing XSS is ok even if It breaks things (security is always no1).

Anyways I think someone else should try, IU had my fair chances and it didn't worked out

avatar dgrammatiko dgrammatiko - change - 12 Mar 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-03-12 11:08:38
Closed_By dgrammatiko
avatar dgrammatiko dgrammatiko - close - 12 Mar 2022
avatar dgrammatiko
dgrammatiko - comment - 12 Mar 2022

@HLeithner for reference I saw it coming and tried to fix things here: #33773 but people were against it

avatar HLeithner
HLeithner - comment - 12 Mar 2022

ok but why do you close this PR if you only write in the title that you fix the math.div deprecation?
If it's a security thing you should report it to the jsst.

avatar dgrammatiko
dgrammatiko - comment - 12 Mar 2022

If it's a security thing you should report it to the jsst.

There's nothing to report again, I raised my concerns when the choices was introduced to J4. Also there is no real B/C break as the default behaviour still allows innerHTML according to their docs:

This behaviour has been deprecated. The new option allowHTML has been introduced, with the current default to true.

Ref: https://github.com/Choices-js/Choices/releases/tag/v10.0.0

avatar HLeithner
HLeithner - comment - 12 Mar 2022

I would say that's a b/c?

As a result of this change, callbackOnCreateTemplates now receives the full configuration object, instead of just classNames. The method signature to match previous versions is now ({ classNames }, data). See the documentation for the updated example.

avatar dgrammatiko
dgrammatiko - comment - 12 Mar 2022

I would say that's a b/c?

Did you or anyone else used the callback? Theory says yes this is B/C break, practically there are close to 0% changes that someone will be affected by this, or any of the changes due to the upgrade

avatar HLeithner
HLeithner - comment - 12 Mar 2022

that's the problem I can't say it so I can only read the b/c and then I can say we are using semver for js or not.
It's not possible for me to check all dependencies if a b/c break would happen for us or any of our users.

But as always please join the discussion if and where Joomla should use semver joomla/rfc#29

if you think semver is (partly) a bad idea then please share your opinion and we may remove semver for Joomla if this fit better the needs for our users and us.

But just making exception everywhere doesn't make sense that mean we don't make semver but only saying we do and break things expected for users. If it's a security thing then the jsst have to solve it (or course with your help).
The solution can be we update to 10. or backport the fix.

avatar dgrammatiko
dgrammatiko - comment - 12 Mar 2022

The solution can be we update to 10. or backport the fix.

There's another option:

  • Remove the upgrade to v10
  • Use own SCSS files for the choices (as I did in the other PR before upgrading)
avatar HLeithner
HLeithner - comment - 12 Mar 2022

tbh I don't understand what you mean? removing choices? and what has it todo with the scss in this case?

avatar dgrammatiko
dgrammatiko - comment - 12 Mar 2022

@HLeithner check #37255

Also have some tests and either merge it or if it goes out of sync close it. I spent more time that I'm comfortable in this one.

Thanks

avatar HLeithner
HLeithner - comment - 12 Mar 2022

Thanks, I reported the XSS to JSST in your name.

Add a Comment

Login with GitHub to post a comment