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

index: use Node's CommonJS JSON loader #111

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Arcitec
Copy link

@Arcitec Arcitec commented Sep 10, 2024

  • Node natively supports loading JSON via require() since 2011, which is a cleaner and easier way to load JSON:

https://nodejs.org/en/blog/release/v0.5.2

  • Node's require() safely performs regular JSON decode whenever it detects a ".json" file extension, which is what our "detectable.json" filename is using.

  • The createRequire() API is only necessary because arRPC is not a CommonJS project. Using createRequire() is a completely normal, supported interoperability feature of Node, and is documented here:

https://nodejs.org/api/module.html#modulecreaterequirefilename

  • This change was done to help downstream projects based on CommonJS or ASAR bundles. Without this change, arRPC will not work in certain downstream apps (depending on their bundling methods).

@Arcitec
Copy link
Author

Arcitec commented Sep 10, 2024

This is ready to merge. Everything works exactly as before.

The ./detectable.json path means "relative to this source code file". For that reason, there is no longer any need to calculate the path to the current source file either.

It's a much cleaner way to load JSON, by relying on Node's native support for it. Especially if we change the project to "type": "commonjs" later, as we could then even remove the two "require" function lines.

@CanadaHonk
Copy link
Contributor

This is just a change to change? I don't see any benefit since the project will never move to commonjs.

@Arcitec
Copy link
Author

Arcitec commented Sep 10, 2024

This is just a change to change? I don't see any benefit since the project will never move to commonjs.

It's a change to help downstream apps that integrate arRPC, so that they don't need to patch that command when they include arRPC in ASAR bundles or something like that. The detectable.json path does not exist on disk in those cases, so the old code was not working for them.

With the change to use require(), the code now works for downstream bundles without them needing to patch it anymore:

https://github.com/Vencord/Vesktop/blob/main/patches/arrpc%403.4.0.patch

And the way that I'm including require() via createRequire() means that it's the exact same internal Node require() implementation. So this change readies arRPC for much easier use downstream. I saw their patch and rewrote it in the correct way that now works universally, no matter if arRPC runs as an unpacked module or as an ASAR or as part of a CommonJS project etc etc.

@Arcitec
Copy link
Author

Arcitec commented Sep 14, 2024

@CanadaHonk Hi Oliver, I noticed that you're very busy with other projects. However, since this is such a small fix, I'm wondering if we can move ahead with this so that I can then alert the downstream project that they don't need to patch arRPC any longer. :)

@hUwUtao
Copy link

hUwUtao commented Sep 23, 2024

as i pretty much not like node to run in background, i have an idea to run it with bun. ive tried it just work normally.
i have an idea where we can bundle into a single bun executable. though, it wasnt good at dealing with files when it wrapped with __dirname. so i think the optimal way is to just put like this and casually update the process.

@Arcitec
Copy link
Author

Arcitec commented Sep 25, 2024

@hUwUtao @CanadaHonk Yes, exactly. We need to merge this otherwise arRPC doesn't work when bundled (such as inside ASAR packages). The old code assumes that arRPC is always extracted as regular files on disk. This is not how most apps operate. They bundle things into 1 archive.

update_db.js Outdated Show resolved Hide resolved
- Node natively supports loading JSON via `require()` since 2011, which is a cleaner and easier way to load JSON:

https://nodejs.org/en/blog/release/v0.5.2

- Node's `require()` safely performs regular JSON decode whenever it detects a ".json" file extension, which is what our "detectable.json" filename is using.

- The `createRequire()` API is only necessary because arRPC is not a CommonJS project. Using `createRequire()` is a completely normal, supported interoperability feature of Node, and is documented here:

https://nodejs.org/api/module.html#modulecreaterequirefilename

- This change was done to help downstream projects based on CommonJS or ASAR bundles. Without this change, arRPC will not work in certain downstream apps (depending on their bundling methods).
@Arcitec
Copy link
Author

Arcitec commented Jan 15, 2025

@CanadaHonk Thanks again for discovering the caching issue with update_db.js. Really nice catch! That file has been reverted via force-push now. ❤️

@Arcitec Arcitec requested a review from CanadaHonk January 15, 2025 18:06
@hUwUtao
Copy link

hUwUtao commented Jan 16, 2025

no, bundling will eventually mess up the dynamic trait of detectable.json db. fs is still the way, but to needed handle the file of cache somewhere (imo tempdir). also, could be done to implement the updater in standalone process.
nodejs fs still so widely implemented across runtime. but if needed an bundler-agnostic, runtime-agnostic, import() is the correct way, and it doesnt mess up the nature of esm, any bundler

@Arcitec
Copy link
Author

Arcitec commented Jan 16, 2025

@hUwUtao Unfortunately your comment is nonsense. Bundling is not supposed to dynamically update detectable.json. You are not supposed to bundle update_db.js. You will bundle a static detectable.json file. And when you want to update it, you build a new bundle as a developer and release a new version of your app.

There is no mechanism for dynamically loading detectable.json from another location on disk. It always lives relative to the rest of the bundled files, inside the ASAR package. Which cannot be updated (rewritten) by your app. The only way to update it is via app updates.

Furthermore, arrpc was designed to never dynamically update detectable.json while people are using it. The static database is one of its "features".

Now, if there is a scenario where you might NOT be bundling arrpc inside your ASAR and you want to reload a newer detectable.json, then you have to use all kinds of tricks to reload arrpc anyway (because arrpc will not reload the JSON contents after arrpc itself has already been loaded). And then you can mess with the require.cache object to delete the old JSON cache. But that's not within the scope of arrpc's project.

@hUwUtao
Copy link

hUwUtao commented Jan 16, 2025

to "ship" and to "bundle" is 2 different level. i can see that the way we do is pretty orient on embed the whole thing. as for statically bundle it, there is no reason to depend on node require call, as bundler will eventually treat json as js and no parse done, if all was just for stuffing a json in asar package, well smh

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.

3 participants