Skip to content

feat: cwriter for wasm v7 - #133

Open
Icemap wants to merge 1 commit into
vbauerster:v7from
Icemap:feat-cwriter-for-wasm-v7
Open

feat: cwriter for wasm v7#133
Icemap wants to merge 1 commit into
vbauerster:v7from
Icemap:feat-cwriter-for-wasm-v7

Conversation

@Icemap

@Icemap Icemap commented Nov 10, 2023

Copy link
Copy Markdown

No description provided.

@wanshicheng

Copy link
Copy Markdown

Your commits does not support GOOS=wasip1 and GOARCH=wasm

@Solaris-star Solaris-star left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a superset of #132 — it adds the same WASM stubs plus util_wasm.go for the ioctl constant. Two issues:

  1. Build tags: uses the old //+build syntax. Go 1.17+ prefers //go:build. #132 already has the correct syntax — worth aligning.

  2. IsTerminal returns true: same concern as #132 — WASM has no terminal, so false is more accurate. Callers checking IsTerminal to decide between TTY and plain output would get the wrong branch.

Recommend consolidating #132 and #133 into a single PR with //go:build tags and IsTerminal → false. The core approach (dummy Writer, no-op GetSize) is correct.

@Solaris-star

Copy link
Copy Markdown
Contributor

I rechecked the current head b8e104015e35ebecc712cbf6cc585eee86cfd97e from a clean checkout.

There is a compile blocker for the target platform:

  • go test ./... ✅ passed on the PR checkout (the default-platform cwriter package reports [no tests to run]).
  • GOOS=js GOARCH=wasm go test ./cwriter ❌ fails at cwriter/writer.go:55: w.clearLines undefined.

Flush calls w.clearLines() whenever w.lines > 0; that method is supplied by writer_posix.go and writer_windows.go, both excluded by the new WASM build constraint. writer_wasm.go currently defines GetSize and IsTerminal but not the required (*Writer).clearLines method. Please add a WASM-safe no-op (or otherwise provide the platform implementation) and rerun the cross-compile test. I did not test browser/runtime behavior; this is a clean-checkout compile result at the SHA above.

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