Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump version of jQuery to 3.6.4 & updated ref links #8909

Merged
merged 1 commit into from Mar 28, 2023
Merged

Conversation

auvipy
Copy link
Member

@auvipy auvipy commented Mar 16, 2023

No description provided.

@auvipy auvipy requested a review from a team March 16, 2023 11:05
@baseplate-admin
Copy link
Contributor

Hey forgive me for asking but can't we link against django admin's jQuery?

@auvipy
Copy link
Member Author

auvipy commented Mar 24, 2023

Hey forgive me for asking but can't we link against django admin's jQuery?

that's a good question! DRF uses twitter bootstrap which django admin doesn't. I followed the historical approach here. But if old maintainer of the project suggest for using django admins jquery I will update accordingly. another point to consider here is that, the latest version of twbs has stopped using jquery & switched to vanilla JS. as DRF will also upgrade it's twbs version, it most likely will drop usage of jquery

@auvipy auvipy marked this pull request as ready for review March 24, 2023 06:43
@auvipy
Copy link
Member Author

auvipy commented Mar 24, 2023

@tschwaerzl did you notice any regression in existing UI's??

@baseplate-admin
Copy link
Contributor

baseplate-admin commented Mar 25, 2023

But if old maintainer of the project suggest for using django admins jquery I will update accordingly

This is a good answer. Thanks :D


Should i adress this in a Issue that can get the maintainers attention or should i ping here?

@auvipy
Copy link
Member Author

auvipy commented Mar 25, 2023

I am also a maintainer now for your kind info :)

@kevin-brown
Copy link
Member

kevin-brown commented Mar 25, 2023

I'm fairly certain that Django is also in the process of dropping the dependency on jQuery so I don't think it makes sense to switch over to that one at this time. I'm fine updating our vendored version to a newer one, but we should also look towards removing the dependency in the future.

I'm pretty sure we'd also have to take a new dependency against Django's admin, which I don't think we otherwise require.

@auvipy
Copy link
Member Author

auvipy commented Mar 25, 2023

I think we should only focus on DRF vendroed version for now as this fix some security / other bugs. and for future, moving away from jquery

@baseplate-admin
Copy link
Contributor

baseplate-admin commented Mar 28, 2023

I'm fairly certain that Django is also in the process of dropping the dependency on jQuery so I don't think it makes sense to switch over to that one at this time.

I am fairly certain thats not happening The reasoning is that too many dependencies require jQuery to be present in django admin ( eg: django-hstore-fields)

I think we should only focus on DRF vendroed version for now as this fix some security / other bugs. and for future, moving away from jquery

Fair enough. I respect your opinion.

@tschwaerzl did you notice any regression in existing UI's??

If my opinion matters. No i didn't find any regression during my initial ( albeit very light testing )

I am also a maintainer now for your kind info :)

Apologies. I was not aware of this. Forgive my rude response from before. ( Congratulations !! )


Since django also updated their jquery to 3.6.4 i think we can move with this.

@tschwaerzl
Copy link

No issues found. Everything working as it should be.

@auvipy
Copy link
Member Author

auvipy commented Mar 28, 2023

Apologies. I was not aware of this. Forgive my rude response from before. ( Congratulations !! )

never mind my comment was purely pun intended! no offense taken! you views are highly appreciated. the only reasoning here against using django jquery is, DRF is based on twbs. that's the issue. If you guys can confirm no regression happening and also check the version file in this PR is security checked then I can merge this.

@auvipy auvipy added this to the 3.15 milestone Mar 28, 2023
@auvipy auvipy merged commit 29b6dd8 into encode:master Mar 28, 2023
7 checks passed
@auvipy auvipy deleted the jq branch March 28, 2023 09:43
@auvipy
Copy link
Member Author

auvipy commented Aug 30, 2023

we have another followup PR #9094

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

Successfully merging this pull request may close these issues.

None yet

5 participants