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

WIP: Make examples ESM and add ESM to node-ble #422

Merged

Conversation

vilicvane
Copy link
Contributor

  • Changed package.json type for both matter-node-ble.js and matter-node.js-examples to "module".
  • Added missing .js file extension for matter-node-ble.js.
  • Added require that works in both CJS and ESM using Node.js package.json. Other better ways?

@vilicvane vilicvane force-pushed the feature/examples-and-node-ble-to-esm branch from c313555 to d075351 Compare October 20, 2023 14:17
@Apollon77
Copy link
Collaborator

I need to check the exports on a big screen (just mobile right now) but yes that require thingy seems to be that way as only available option. Will check latest tomorrow.

@vilicvane
Copy link
Contributor Author

Don't know, it actually surprised me that there's little discussion related to this.

I am thinking about importing the dual format require with relative path as well, what do you think?

@vilicvane vilicvane marked this pull request as draft October 21, 2023 15:30
@Apollon77
Copy link
Collaborator

@vilicfor the "require": We could also simply make the "loading"methdod async and use await import?

@vilicvane
Copy link
Contributor Author

@Apollon77 Yes but that would introduce noticeable changes. Currently both bleno and noble usages are synchronized, as top level await is only supported in esm (actually relatively recent esm).

@Apollon77
Copy link
Collaborator

Apollon77 commented Oct 21, 2023

wait ... look deeper ... bleno and noble requires ia both "lazy" in a method, e,.g. https://github.com/project-chip/matter.js/blob/main/packages/matter-node-ble.js/src/ble/BlenoBleServer.ts#L31 so it is not top level :-)

so when we make this "Initialize" method async then all should be fine.

PS: Yes because initialize is done in constrzuctore we would needs to adjust that a bit, but like the "create" methods we use elsewhere for the same reasons

@vilicvane
Copy link
Contributor Author

Yes, I mentioned top-level await because if that could be done, there's nothing need to change elsewhere.

And asynchronousness is contagious, I took some reference looking up, and there might be a tree to update. I'll do whatever you want. ;)

@lauckhart
Copy link
Collaborator

Yes, I mentioned top-level await because if that could be done, there's nothing need to change elsewhere.

I think given the migration to ESM top-level await is fine, desirable even in this case.

@Apollon77
Copy link
Collaborator

SO tehe question is still if we should use this "extra exposed require" or if we just switch the "Inititialize" methdos to async and use an await import there ... then we do dnot need this requiets special stuff

@lauckhart
Copy link
Collaborator

Switching to async initialization would be ideal but I'd leave up to @vilic since he's doing the work. :) Could also accept with the require and add a TODO for swapping to async.

@vilic FWIW we've dealt with "cascading async refactors" before and they aren't usually too terrible since a lot of our initialization code is already async.

@vilicvane vilicvane force-pushed the feature/examples-and-node-ble-to-esm branch from d075351 to 1760e6b Compare October 31, 2023 15:00
@vilicvane
Copy link
Contributor Author

@lauckhart I'll leave the PR as-is then. 😄

I just tried the "cascading async refactor" (nearly finished) but as it changes some API (e.g. Ble) and I don't feel good changing the API for compatibility with legacy versions of runtimes.

Also, there are some issues with Bleno initialization refactor that might require putting things in different places which I think would require aligning with your preferences.

I think it's better to be done by you guys so you can make sure there's no unnecessary compromises.

@vilicvane vilicvane marked this pull request as ready for review October 31, 2023 15:14
@Apollon77 Apollon77 merged commit 9427879 into project-chip:main Oct 31, 2023
17 checks passed
@JimBuzbee
Copy link
Contributor

I know it's a bit late :-) But "npm run bundle-device" in the examples directory no longer works:

[ERROR] Could not resolve "dist/cjs/examples/DeviceNode.js"

Is that a result of this change?

@vilicvane
Copy link
Contributor Author

vilicvane commented Nov 7, 2023

@JimBuzbee Seems to be caused by this PR. You may temporarily fix this by replacing all cjs in packages\matter-node.js-examples\package.json with esm. Will submit another PR to fix.

Turned out to be a little bit more complicated than I thought. esbuild does not support either polyfilling import.meta.url for cjs nor polyfilling require() for esm... Still trying to find a way.

@vilicvane
Copy link
Contributor Author

@JimBuzbee I have submitted a PR fixing this, please check it out.

@Apollon77
Copy link
Collaborator

Fix released as new Nightly

@JimBuzbee
Copy link
Contributor

@vilicvane - Thanks for the quick response - works for me now.

@Apollon77
Copy link
Collaborator

Next nightly is also on it's way, sorry

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.

4 participants