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

io: change Reader interface #2591

Merged
merged 8 commits into from Jul 6, 2019

Conversation

6 participants
@kt3k
Copy link
Contributor

commented Jun 27, 2019

This PR changes the Reader interface from:

export interface Reader {
  read(p: Uint8Array): Promise<ReadResult>;
}

to:

export interface Reader {
  read(p: Uint8Array): Promise<number | EOF>;
}

I introduced EOF symbol according to @bartlomieju's comment and the discussion in denoland/deno_std#444


closes #2384

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@kt3k you need to update /tools/deno_tcp.ts - it's still referencing ReadResult.eof

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2019

@kt3k Cool! This is going to make v0.11 bump in std painful. We should start a PR over there before the release.

@kt3k

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

@bartlomieju Thank you! 👍

@ry OK. I'll start it soon!

@kt3k

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

ah, http_benchmark seems depending on std/http, std/io, etc.

error TS2694: Namespace 'Deno' has no exported member 'ReadResult'.
► file:///home/travis/build/denoland/deno/js/deps/https/deno.land/std/io/bufio.ts:7:24
7 type ReadResult = Deno.ReadResult;
                         ~~~~~~~~~~

Now I'm working on deno_std side kt3k/deno_std@fcda701

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2019

@kt3k you should be able to update the submodule in js/deps/https/deno.land/std to your branch to be able to test.

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2019

👍🏻👍🏻👍🏻

@kt3k kt3k force-pushed the kt3k:feature/refactor-reader branch from 4bcec78 to 5b3ed57 Jun 30, 2019

kt3k added some commits Jun 30, 2019

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

@kt3k This looks good to me. Is this branch ready to land? It's a bit of a tricky situation with the deno_std upgrades...

@kt3k

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

@ry
I think it's necessary to land and release this first for passing the test of deno_std's PR. In that case, the next version doesn't work with std until the deno_std's updates landed.
That's not ideal, but in my view, we don't have better way to update this kind of things with current workflow.

I think that If we have something like beta release of deno (which only developers can access) and beta branch of deno_std, we can do more graceful updates in this kind of situation.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

@kt3k latest Deno release will work with latest release of standard lib. I think it's fine if there's a minor break on master branches.

I think that If we have something like beta release of deno (which only developers can access) and beta branch of deno_std, we can do more graceful updates in this kind of situation.

-1 for now, since we're still not past 1.0 seems like a waste of resources

@kt3k

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

@ry
I still have a little concern about @ts-ignore which I put in //tools/deno_http_proxy.ts. Please wait to land for a while.

@bartlomieju

-1 for now, since we're still not past 1.0 seems like a waste of resources

I agree with this. Maybe it's better to have such workflow after 1.0.

@kt3k kt3k closed this Jun 30, 2019

@kt3k kt3k reopened this Jun 30, 2019

// TODO(kt3k): lib.deno_runtime.d.ts has 2 EOF types: Deno.EOF and io.EOF.
// They are identical symbols, and should be compatible. However typescript
// recognizes they are different types and the below call doesn't compile.
// @ts-ignore
req.respond(resp);

This comment has been minimized.

Copy link
@ry

ry Jul 1, 2019

Collaborator

That's annoying.

@kitsonk can you advise us?

This comment has been minimized.

Copy link
@kt3k

kt3k Jul 1, 2019

Author Contributor

@kitsonk
I have trouble with this line. The error is like the below:
スクリーンショット 2019-07-02 0 01 35

lib.deno_runtime.d.ts of this branch contains two EOF unique symbol declarations. (This seems strange because in this branch EOF is defined only once, but .d.ts treat it as different types depending on where it is used.)

I tried to move EOF to several other places (js/eof.ts, js/deno.ts, etc) to avoid this, but I couldn't find a solution.

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Jul 1, 2019

Contributor

Probably doing something in deno_std like

export const EOF: unique symbol = Deno.EOF || Symbol("EOF");
export type EOF = typeof EOF;

?
(I did not follow the context of this discussion so I might be wrong)

This comment has been minimized.

Copy link
@kt3k

kt3k Jul 2, 2019

Author Contributor

The problem seems more subtle to me.
This change doesn't include 2 symbol definitions. EOF is defined only in io.ts. deno.ts import it and re-export it. In that case, EOF in io.ts and EOF in deno.ts should be the same thing. But lib.deno_runtime.d.ts (= deno types) includes 2 EOFs under namespace Deno and namespace io. Actually lib.deno_runtime.d.ts includes the entire contents of io.ts twice in Deno and io namespaces. (io namespace is not accessible from the user (maybe). I'm not sure why this exists. Probably a workaround for something.)

lib.deno_runtime.d.ts is created by the special tool //tools/ts_library_builder/. I suspect something goes wrong in it.

This comment has been minimized.

Copy link
@kt3k

kt3k Jul 2, 2019

Author Contributor

Found a workaround for this:

in //js/io.ts

// TODO: Make this symbol type
export const EOF: "EOF" = "EOF";
export type EOF = typeof EOF;

This avoids the above problem.

This comment has been minimized.

Copy link
@kt3k

kt3k Jul 2, 2019

Author Contributor

@j-f1
I think symbol type is ideal for the type of EOF and we need to make it symbol in the future. But for now I don't see any way to declare it as symbol correctly, so I would suggest the above as an alternative path to make things forward.

This comment has been minimized.

Copy link
@kt3k

kt3k Jul 4, 2019

Author Contributor

Found another workaround. If we move EOF to global namespace (window), it seemed working. The diff looks like this ( kt3k@828cf3a )

This comment has been minimized.

Copy link
@ry

ry Jul 4, 2019

Collaborator

@kt3k TBH I don't understand the issue, but I'm fine with that workaround (it's better than making it global). I think we should just move forward with this change and try to clean it up in later. I suspect some changes to ts_library_builder could fix this...

This comment has been minimized.

Copy link
@j-f1

j-f1 Jul 4, 2019

Contributor

Could you do export const EOF: typeof Deno.EOF = Deno.EOF in io.ts?

This comment has been minimized.

Copy link
@kt3k

kt3k Jul 4, 2019

Author Contributor

@ry OK. I'll update the PR to use the workaround.

I suspect some changes to ts_library_builder could fix this.

I agree that this needs some changes to ts_library_builder.

@j-f1
That causes the following error:

python ../../tools/run_node.py ../../node_modules/ts-node/dist/bin.js --project /Users/kt3k/s/deno/tools/ts_library_builder/tsconfig.json --skip-ignore ../../tools/ts_library_builder/main.ts --basePath ../../ --inline ../../js/lib.web_assembly.d.ts --buildPath . --outFile gen/cli/lib/lib.deno_runtime.d.ts --silent --debug
../../js/io.ts:6:26 - error TS4025: Exported variable 'EOF' has or is using private name 'Deno'.

6 export const EOF: typeof Deno.EOF = Deno.EOF;
                           ~~~~
@kt3k

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@ry
I believe this is ready to be merged now. After this released, denoland/deno_std#527 needs to be updated soon. denoland/deno_std#527 passes tests with custom deno of this branch on my machine, so I believe it'll be easy to update.

@ry

ry approved these changes Jul 5, 2019

Copy link
Collaborator

left a comment

@kt3k Sounds good. I expect we might have to adjust this a bit before the release.

I've pushed denoland/deno_std#527 to the branch https://github.com/denoland/deno_std/tree/20190705_update_reader so that it's advertised.

Thanks for pushing through this interface change!

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2019

Changing the EOF symbol to a "const string" may seem smart from a typing perspective, but it's obviously bad in practice.
Imagine you have this:

let line: string | EOF = reader.readLine();
if (line === EOF) { explode() }

What happens if the file contains a line "EOF" ?

@ry
Copy link
Collaborator

left a comment

@piscisaureus makes a good point.

Can't we just put in some hack somewhere where it adds at the beginning of lib.deno_runtime.d.ts

declare Deno.EOF: unique symbol;
export type EOF = typeof Deno.EOF;

(cc @kitsonk )

@kt3k

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@piscisaureus @ry
OK. I see this breaks the case of string | Deno.EOF return type.

Another workaround suggestion. How about making Deno.EOF null type (as temporary solution)? This seems working and doesn't seem affecting current use cases in deno_std. (and it doesn't require the changes of ts_library_builder)

export const EOF: null = null;
export type EOF = null;
@ry

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2019

@kt3k Sounds good

@ry

ry approved these changes Jul 6, 2019

Copy link
Collaborator

left a comment

LGTM

@ry ry merged commit a948f9f into denoland:master Jul 6, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.