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

Move Switchery library to npm/yarn #2242

Merged
merged 1 commit into from Oct 21, 2020

Conversation

McGiverGim
Copy link
Member

Moves the switchery library to the npm/yarn form.

We were using this: https://github.com/abpetkov/switchery
but it seems it is not directly published into npm, so I took one of the forks that seems similar: https://www.npmjs.com/package/switchery-latest

@McGiverGim
Copy link
Member Author

Again, this Karma thing... now the tests are trying to load the GUI, the GUI tries to load the Switchery, it uses require but it is not supported in the Karma browser... I will try to fix it, but maybe someone with more experience with this can help?

@McGiverGim
Copy link
Member Author

Finally it seems the require was not necessary, it supports a browserify version out of the box 🤦‍♂️

@@ -66,7 +67,7 @@
<script type="text/javascript" src="./node_modules/three/examples/js/renderers/Projector.js"></script>
<script type="text/javascript" src="./js/libraries/jquery.flightindicators.js"></script>
<script type="text/javascript" src="./node_modules/semver-min/umd/index.js"></script>
<script type="text/javascript" src="./js/libraries/switchery/switchery.js"></script>
<script type="text/javascript" src="./node_modules/switchery-latest/dist/switchery.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Changing this to type="module" should alleviate the in-browser woes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, but I can't see the difference.

The library has two version, one in the root of the folder, that I think is a pure module, and the other in the dist folder that I think is a browserify version of it.

Sorry, all of this is new to me. I have read a little about modules, but I can't see all the implications until I use it more...

Copy link
Member

Choose a reason for hiding this comment

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

The comment overlapped with your latest change.
Using the browserified version (where available) as text/javascript will work. Using the non-browserified version as module will work as well. Not absolutely sure which one is better, but my hunch is to use the module version for all modules, as this can be done consistently.

Also, your current version has got two type= attributes. 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

True 🙀
I will do some tests tomorrow to confirm your thoughts.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mikeller finally I ended letting the browserify version. If I change to the module version:

  • We need to use require (the node version for module import) because the import can only be used into another module, so we need to start migrating our others js to modules if we want to use import. With require it works.
  • We need to change our Karma tests to be executed in node and not in browser. I have seen that this can be accomplished using Jasmine as Karma framework. If not it does not understand the require. Maybe it can understand the import.

So for this PR I think is better to make the things simple and let this changes for a future PR when we start to migrate our js to modules.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's fair enough for now - we can do the migration to consumption of resources as modules at a later stage.

@sonarcloud
Copy link

sonarcloud bot commented Oct 21, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mikeller mikeller merged commit 3d85c87 into betaflight:master Oct 21, 2020
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.

None yet

2 participants