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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use jquery package #3540

Merged
merged 1 commit into from Aug 16, 2023

Conversation

chmelevskij
Copy link
Member

Use jquery as npm module instead of global version where possible.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@McGiverGim
Copy link
Member

If this is going to be included in the next version I think we need to merge this ASAP. It can produce errors if some import is forgotten that can be difficult to detect until tested.

@chmelevskij
Copy link
Member Author

hey, this will be my focus tomorrow. It's one of the pre-cursors for whole web work I'm doing so want to get it in ASAP as well.

Everything seems to be working fine, just have few tabs where some of the jquery plugins we use don't register properly 🤷

@chmelevskij chmelevskij marked this pull request as ready for review August 13, 2023 09:52
Comment on lines +138 to +140
"resolutions": {
"jquery": "3.6.3"
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to prevent clash of different jquery versions when plugins are loaded in.

@github-actions

This comment has been minimized.

haslinghuis
haslinghuis previously approved these changes Aug 13, 2023
@haslinghuis
Copy link
Member

Testing now (fails on tab led strip):

Uncaught TypeError: jqueryExports(...).selectable is not a function
    at HTMLDivElement.process_html (led_strip.js:292:24)
    at HTMLDivElement.<anonymous> (jquery.js:10233:14)
    at Function.each (jquery.js:383:19)
    at jQuery.fn.init.each (jquery.js:205:17)
    at Object.<anonymous> (jquery.js:10232:9)
    at fire (jquery.js:3213:31)
    at Object.fireWith [as resolveWith] (jquery.js:3343:7)
    at done (jquery.js:9617:14)
    at XMLHttpRequest.<anonymous> (jquery.js:9878:9)

@haslinghuis haslinghuis dismissed their stale review August 13, 2023 10:37

Test failing

@sonarcloud
Copy link

sonarcloud bot commented Aug 13, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 21 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@blckmn
Copy link
Member

blckmn commented Aug 13, 2023

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> PASS
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's test it!

@haslinghuis haslinghuis merged commit 40c243f into betaflight:master Aug 16, 2023
9 checks passed
@nerdCopter
Copy link
Member

Let's test it!

i guess that meant merge it and let the pilots find out 😆

@haslinghuis
Copy link
Member

😂 #3540 (comment)

@ASDosjani
Copy link
Contributor

i guess that meant merge it and let the pilots find out 😆

This pilot found out that it breaks master😂

@haslinghuis
Copy link
Member

Just tested master and it works fine.

@ASDosjani
Copy link
Contributor

Sorry, it had old packages, I run yarn and now it's working for me as well.

@HThuren
Copy link
Member

HThuren commented Jan 6, 2024

hi @chmelevskij ,
I will like to use multiple-select-locale-all.min.js insted of multiple-select.min.js
The old way in main.html:
<script type="text/javascript" src="./node_modules/multiple-select/dist/multiple-select.min.js"></script>
<script type="text/javascript" src="./node_modules/multiple-select/dist/multiple-select-locale-all.min.js"></script>
are now changed to jqueryPlugins.js:
import 'multiple-select';

but how are this resolved, how do I change to use multiple-select-locale-all ?

@chmelevskij
Copy link
Member Author

@haslinghuis have you tried updating import to import 'multiple-select-locale-all' in that file?

@HThuren
Copy link
Member

HThuren commented Jan 10, 2024

@haslinghuis have you tried updating import to import 'multiple-select-locale-all' in that file?

I have a ongoing test, working on this output:
Uncaught TypeError: Failed to resolve module specifier "multiple-select-locale-all". Relative references must start with either "/", "./", or "../".

@HThuren
Copy link
Member

HThuren commented Jan 10, 2024

maybe a distribution issue

@HThuren
Copy link
Member

HThuren commented Jan 10, 2024

@chmelevskij , only first of below three works (?)

import 'multiple-select'
import 'multiple-select/dist/multiple-select.js'
import 'multiple-select/dist/multiple-select-locale-all.min.js'

@HThuren
Copy link
Member

HThuren commented Jan 11, 2024

@chmelevskij @haslinghuis can you explain the resolve / point to documentaion of how elementes in jQueryPlugins.js are resolved, what are in use in the imported module, ie. package.json or ??

package.json in multiple-select contain:

  "name": "multiple-select",
  "main": "dist/multiple-select.min.js",
  "module": "dist/multiple-select-es.min.js",

@nerdCopter
Copy link
Member

nerdCopter commented Jan 11, 2024

  • all packages maintained in package.json (via yarn add --save[-dev]). yarn install is a prerequisite to usage/execution.
  • code author decides when to use package via import.
  • minified versions are some effect of packaging process.
  • if i recall correctly, author can import entire package, or individual sub-command.
  • ./dist/ is a result of execution/packaging.
  • git clean -dxf to remove all cruft not included in repo.
$ grep -Iin multiple ./package.json 
63:    "multiple-select": "^1.6.0",

Other devs will know more. -- and i'm open to corrections.

Should "Discussions" be enabled on Betaflight Github? or maybe too many distractions.

@haslinghuis
Copy link
Member

Try multiple-select-locale-all.min.js instead in main.html (line 51)

<link type="text/css" rel="stylesheet" href="./node_modules/multiple-select/dist/multiple-select.min.css" media="all"/>

@chmelevskij
Copy link
Member Author

@haslinghuis let's not go back to script tags, that will cause pain later on down the line with building for web.

RE: how imports are resolved and package json

https://nodejs.org/docs/latest-v20.x/api/packages.html#main-entry-point-export
https://nodejs.org/docs/latest-v20.x/api/packages.html#main-entry-point-export

import 'multiple-select/dist/multiple-select-locale-all.min.js' this seems like this should work 🤔

@HThuren
Copy link
Member

HThuren commented Jan 14, 2024

@chmelevskij
Problem moved, now the modules are imported (remember to delete cache and debug catalogs), but an undeclared variable give exeception
image
but this happen before main window are fully open, so debug are difficult (for me)

Have a look in #3756

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

None yet

7 participants