Inject deno.arch/deno.platform from Node's process.arch/process.platform through rollup #773
Conversation
kevinkassimo
force-pushed
the
platform/rollup
branch
from
a9cc66a
to
5fda656
Compare
rollup.config.js
Outdated
| code: `export const arch = "${process.arch}"; | ||
| export const platform = "${process.platform}"; | ||
| `, | ||
| map: { mappings: "" } |
There was a problem hiding this comment.
I think it is fine for now, but we might want a todo to actually have a valid map in here, as it might cause confusing stack traces. It may be worth testing it to just see how bad it is at the moment.
There was a problem hiding this comment.
So I just ripped off the sourceMap generating code from rollup-plugin-inject. Hopefully it could work
js/platform.ts
Outdated
| @@ -0,0 +1,3 @@ | |||
| // Dummy. Injected in rollup.config.js | |||
| export const arch = ""; | |||
There was a problem hiding this comment.
maybe something other than empty string? Like "unknown"?
There was a problem hiding this comment.
Also, just thinking out loud, is that when these types get extracted and bundled up, because this is a const, TypeScript I suspect will create a literal type of "" for these, which isn't the best. What would be better would be to have a literal union type in here so that when it gets bundled into the types, end users have a valid type they can use, like:
export const arch: "x32'" | "x64" | "unknown" = "unknown";
export const platform: "darwin" | "linux" | "win32" | "unknown" = "unknown";There was a problem hiding this comment.
Yeah I did notice this behavior when trying to add basic tests. I can definitely add the union type, just need to update the type if node will be supported on other new platforms (fuschia?)
|
@ry just for source map. It comes as dependency already for some other rollup plugins in |
- Add atob() btoa() denoland#776 - Add deno.arch deno.platform denoland#773 - Add deno.symlink() and deno.symlinkSync() denoland#742 - Add deno.mkdir() and deno.mkdirSync() denoland#746 - Add deno.makeTempDir() denoland#740 - Improvements to FileInfo interface denoland#765, denoland#761 - Add fetch.blob() - Upgrade V8 to 7.0.276.15 - Upgrade Rust crates
Fixes #16922. The error messages in the `ffi` module are somewhat cryptic when passing functions that have invalid `parameters` or `result` type strings. While the generated serializer for the `ForeignFunction` struct correctly outputs a correct and verbose message, the user sees a far less helpful `data did not match any variant` message instead. The underlying cause appears to be the fallback message in the auto-derived deserializer for untagged enums [1] generated as a result of `ForeignSymbol` being marked as `#[serde(untagged)]` [2]. Passing an unexpected value for `NativeType` causes it to error out while attempting to deserialize both enum variants -- once because it's not a match for the `ForeignStatic` variant, and once because the `ForeignFunction` deserializer rejects the invalid type for the parameters/return type. This is currently open as [serde #773](serde-rs/serde#773), and not a trivial exercise to fix generically. [1] https://github.com/serde-rs/serde/blob/v0.9.7/serde_derive/src/de.rs#L730 [2] https://github.com/denoland/deno/blob/main/ext/ffi/dlfcn.rs#L102 [3] serde-rs/serde#773 Note that the auto-generated deserializer for untagged enums uses a private API to buffer deserializer content that we don't have access to. Instead, we can make use of the `serde_value` crate to buffer the values. This can likely be removed once the official buffering API lands (see [4] and [5]). In addition, this crate pulls in `serde_json` as a cheap way to test that the deserializer works properly. [4] serde-rs/serde#741 [5] serde-rs/serde#2348
Related to #771 .
Now
deno.archanddeno.platformbecomes exactly the same as Node'sprocess.archandprocess.platform. Injected compile-time inrollup.config.js.(I am not familiar with rollup but a brief examination of its documentation seems justifying this solution, though might not be the best . Manually tested and working locally. Hard to create test for it though...)
@kitsonk