Skip to content

WIP: Add implementation for server.url - #6064

Closed
GabrielDougherty wants to merge 11 commits into
oven-sh:mainfrom
GabrielDougherty:get-url
Closed

GabrielDougherty wants to merge 11 commits into
oven-sh:mainfrom
GabrielDougherty:get-url

Conversation

@GabrielDougherty

Copy link
Copy Markdown

What does this PR do?

This adds a new url property on server which returns a URL object. That way, the URL object can be passed around instead of the individual fields server.hostname and server.port.

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

I wrote automated tests

If JavaScript/TypeScript modules or builtins changed:

  • I ran make js and committed the transpiled changes
  • I or my editor ran Prettier on the changed files (or I ran bun fmt)
  • I included a test for the new code, or an existing test covers it

If Zig files changed: (TODO)

  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
  • I or my editor ran zig fmt on the changed files
  • I included a test for the new code, or an existing test covers it
  • JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed

If new methods, getters, or setters were added to a publicly exposed class:

  • I added TypeScript types for the new methods, getters, or setters

If *.classes.ts files were added or changed:

  • I ran make codegen to regenerate the C++ and Zig code

@GabrielDougherty
GabrielDougherty marked this pull request as draft September 26, 2023 05:44
@paperclover

Copy link
Copy Markdown
Contributor

the url struct you found it something we shouldn't be using for this; it's our own url parser in zig, which has some advantages in some use cases (doesn't allocate), but it has alot of issues since it wont normalize.

another issue of creating a separate generated class here is that any usage of it from js won't be instanceof globalThis.URL.

the approach you should take is what you mentioned in the issue: have a binding to JSC (WTF::URL and WebCore::JSDOMURL). I imagine this could look like URL__toJS(), and use the bun.JSC.URL (bindings.zig) struct.

as a final nit, i think cache: true, on getURL should be set (in the classes.ts file). we should have a test that server.url === server.url (calling getter twice)

@GabrielDougherty

Copy link
Copy Markdown
Author

Ok, I will rework this PR to bind to JSC and implement your other suggestions.

@GabrielDougherty

Copy link
Copy Markdown
Author

Hi @paperdave, I understand from the Development documentation that if I add JSC.markBinding(@src()); I am supposed to call make headers. But when I run make headers I get the following error:

zig build-obj headers Debug native-native-gnu.2.27: error: the following command failed with 1 compilation errors:
/home/gabriel/.bun/install/global/node_modules/@oven/zig-linux-x64/zig build-obj -freference-trace=16 /home/gabriel/repos/bun/src/bindgen.zig -lc++ -lc -fno-strip --eh-frame-hdr --emit-relocs -ffunction-sections --cache-dir /home/gabriel/repos/bun/zig-cache --global-cache-dir /home/gabriel/.cache/zig --name headers -fno-compiler-rt -fno-stack-check -target native-native-gnu.2.27 -mcpu znver2-adx-clwb-clzero-f16c-fma-mwaitx-rdpid-rdrnd-rdseed-sha-wbnoinvd-xsavec-xsaveopt-xsaves --mod build_options::/home/gabriel/repos/bun/zig-cache/c/bc52644c36b4613bd3ce1557f7d7c620/options.zig --mod async_io::/home/gabriel/repos/bun/src/io/io_linux.zig --deps async_io,build_options --main-pkg-path /home/gabriel/repos/bun --listen=- 
Build Summary: 1/4 steps succeeded; 1 failed (disable with --summary none)
headers-obj transitive failure
├─ install generated to headers.o transitive failure
│  ├─ zig build-obj headers Debug native-native-gnu.2.27 1 errors
│  └─ zig build-obj headers Debug native-native-gnu.2.27 (+1 more reused dependencies)
└─ zig build-obj headers Debug native-native-gnu.2.27 (+1 more reused dependencies)
src/bun.js/event_loop.zig:348:5: error: dependency loop detected
pub const Task = TaggedPointerUnion(.{
~~~~^~~~~
referenced by:
    EventLoop: src/bun.js/event_loop.zig:604:12
    EventLoop: src/bun.js/event_loop.zig:603:23
    WatchReloader: src/bun.js/javascript.zig:2873:61
    ImportWatcher: src/bun.js/javascript.zig:411:13
    ImportWatcher: src/bun.js/javascript.zig:408:27
    VirtualMachine: src/bun.js/javascript.zig:461:18
    VirtualMachine: src/bun.js/javascript.zig:456:28
    TranspilerJob: src/bun.js/module_loader.zig:235:17
    TranspilerJob: src/bun.js/module_loader.zig:230:31
    Store: src/bun.js/module_loader.zig:245:41
    RuntimeTranspilerStore: src/bun.js/module_loader.zig:193:25
    RuntimeTranspilerStore: src/bun.js/module_loader.zig:189:36
make: *** [Makefile:942: headers] Error 2

Is it absolutely essential for me to get make headers to succeed, in order for the bindings to work?


I am making some progress on re-implementing this PR. I think I implemented URL__toJS( incorrectly, I am currently trying to work out how to get the encoding correct in that function since it is segfaulting right now.

@Electroid Electroid mentioned this pull request Nov 11, 2023
2 tasks done
@GabrielDougherty

Copy link
Copy Markdown
Author

Implemented by other PR

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.

2 participants