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

refactor: use detectProtocolSchemes from @thingweb/td-utils for check… #29

Merged

Conversation

hasanheroglu
Copy link
Contributor

  • I replaced the logic in checkBinding funciton with detectProtocolSchemes from @thingweb/td-utils.
  • I was not able to test since I am not familiar with node-red. Maybe I can get help with that.
  • Closes Refactor binding finder #17

Copy link
Member

@egekorkan egekorkan left a comment

Choose a reason for hiding this comment

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

Looks fine, thanks @hasanheroglu ! @hidetak do you want to review this PR as well?

@hidetak
Copy link
Contributor

hidetak commented Jul 26, 2024

@hasanheroglu @egekorkan

Thank you for the modification to use the function to identify scheme.
However, the following file you modified works on the browser, so it does not work with this modification.
https://github.com/eclipse-thingweb/node-red/pull/29/files#diff-0526a17883259568e0d7d1609bba55d655b6ef96bb18868624fe7a04ec3bb4ce

@hasanheroglu
Copy link
Contributor Author

hasanheroglu commented Jul 29, 2024

Hey @hidetak,

I made some changes, tested it locally, and it seems to work fine.
I added an extra command to the build script of the plugin to copy td-utils library to the resources folder so the library can be accessible in the browser.
The solution may not be very neat, but it was the only one I could come up with.
Let me know what you think.
If you like the solution, before we close this merge request, I can also use the same library to substitute checkBinding functions in nodes.

@hidetak
Copy link
Contributor

hidetak commented Jul 29, 2024

@hasanheroglu

Thank you for your consideration!
It looks like we can directly include web-bundle.min.js from cdn below.
https://cdn.jsdelivr.net/npm/@thingweb/[email protected]/dist/web-bundle.min.js

However, I think the method you modified is better because it can be used in environments where there is no Internet connection or where a proxy is required.

If you like the solution, before we close this merge request, I can also use the same library to substitute checkBinding functions in nodes.

Thank you very much !
You may already know this, but the following may be a description of how to use the js file from a node.
https://nodered.org/docs/creating-nodes/resources

Copy link
Contributor

@hidetak hidetak left a comment

Choose a reason for hiding this comment

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

Thank you for the correction!

@egekorkan egekorkan merged commit 2124b8a into eclipse-thingweb:main Jul 30, 2024
2 checks passed
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.

Refactor binding finder
3 participants