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

Too much usage of mod.ts #130

Closed
ry opened this issue Jan 18, 2019 · 21 comments
Closed

Too much usage of mod.ts #130

ry opened this issue Jan 18, 2019 · 21 comments

Comments

@ry
Copy link
Member

ry commented Jan 18, 2019

I'm glad we've got rid of the usage of index.ts everywhere - however I'm not so happy with the excessive usage of "mod.ts" now. For example, in http/mod.ts:
https://github.com/denoland/deno_std/blob/4283c26b8930ca80e5babca3337b5431f16334d0/http/mod.ts#L1-L7
This is just boilerplate. It would be better to have people link to http/http.ts instead.

I would like to do another disruptive rename - sorry - in which we remove the mod.ts usage.

@hayd
Copy link
Contributor

hayd commented Jan 18, 2019

This would be less boiler plate if there were more modules in http.
Perhaps an easier thing to do is mv http.ts to mod.ts?

I like the mod.ts if there were a bunch of different ts files and mod was gluing them up that, would be useful. Especially since you may need to export functions e.g. say in a utils.ts, but don't really want that to be in the "published" API: mod.ts.

@ry
Copy link
Member Author

ry commented Jan 18, 2019

@hayd What's wrong with a bunch of TS files. Each of them exports different functionality. There's no semantic difference between "a/b/c/mod.ts" and "a/b/c.ts" but the latter is shorter.

@hayd
Copy link
Contributor

hayd commented Jan 18, 2019

top level mod.ts e.g. color/mod.ts then no other mod.ts e.g. color/b/c.ts rather than color/b/c/mod.ts

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Jan 18, 2019

Possibly revisit keroxp's proposal or my proposal?

(Or could I publish this issue to a broader range of developers I know and seek for their ideas?)

@ry
Copy link
Member Author

ry commented Jan 18, 2019

@kevinkassimo I'm more or less onboard with your proposal but I want to avoid having source code in the root - so what if we come up with some categories:

//net/http.ts
//net/irc.ts
//util/testing.ts
//util/asserts.ts
//util/datetime.ts
//util/color.ts

I want us to be very thoughtful about our path names - since it's the public interface. I realize it's disturbing to continually rename stuff, but it's important we iterate on this now rather than later.

@kevinkassimo
Copy link
Contributor

@ry agree. Yeah I think avoid spreading at the root level of deno_std is reasonable.
cc @hayd on the util part since I remember you have some idea on this.

@hayd
Copy link
Contributor

hayd commented Jan 19, 2019

Is everything exported in any file a public interface?
(As I see it that's the benefit of mod.ts, it says "exported things in this file are the public API".)

@keroxp
Copy link
Contributor

keroxp commented Jan 20, 2019

@ry How about like this?
The concepts:

  • no mod.ts and test.ts
  • no duplicated exportations on the project
  • no module files on the root
  • less subdirectories in the module
    • if has, should be the same name with responsible module
    • e.g: module/module_file.ts, module/module_file/some.ts
  • less modules, less files
  • consistent naming of test files
test.ts
http_bench.ts (<- //http/http_bench.ts)
net/ (<- //http)
    file_server.ts
    file_server_test.ts
    http.ts 
    http_test.ts
    http_status.ts
    ws.ts (<- //ws/mod.ts)
    ws_test.ts (<- //ws/test.ts)
    irc.ts (New!)
    irc_test.ts (New!)
fs/
    path.ts ( 
        <- merge: //fs/path/interface.ts,
        <- merge: //fs/path/mod.ts
        <- merge: //fs/path/constants.ts
    )
    path_test.ts (<- merge: //fs/path/*_test.ts)        
io/
    bufio.ts
    bufio_test.ts
    bufio_util.ts (<- ioutil.ts)
    bufio_util_test.ts( <- ioutil_test.ts)
    textproto.ts (<- //textproto/mod.ts)
    textproto_test.ts (<- //textproto/test.ts)
    util.ts 
log/
    log.ts
    log_test.ts (<- test.ts)
    logger.ts
    handler.ts
    levels.ts
util/
    colors.ts (<- //colors/mod.ts)
    colors_test.ts(<- //colors/test.ts)
    datetime.ts (<- //datetime/mod.ts)
    datetime_test.ts(<- //datetime/test.ts)
    sha1.ts (<- //ws/sha1.ts)
    sha1_test.ts (<- //ws/sha1_test.ts)
    flags.ts (<- //flags/mod.ts)
    flags_test.ts (<- //flags/test.ts)
    media_types.ts (<- //media_types/mod.ts)
    media_types/
        db_1.37.0.json (<- //media_types/db_1.37.0.json)
testing/
    testing.ts (<- //testing/mod.ts)
    testing_test.ts (<- //testing/test.ts)

Edited at 2019/01/22:

moved //http/http_proto.ts -> //io/textproto.ts

@ry
Copy link
Member Author

ry commented Jan 20, 2019

textproto should keep its name (it’s not a http parser) but I’m generally in agreement.

@MaxGraey
Copy link
Contributor

MaxGraey commented Jan 20, 2019

May I ask a question about mod.ts instead index.ts?
Style guide mentioned:

index.ts comes with the wrong connotations

What does it mean?
In my opinion index.ts shouldn't not reflect connotation. It has special goal for path's boilerplate and implicitly lead to main directory file. I guess deno_std/http/mod.ts or deno_std/http/http.ts looks less expressive than jsut deno_std/http which possible with index.ts insead mod.ts. Or if deno using own module resolution mechanics may be make sense do same implicit behaviour for mod.ts?

@hayd
Copy link
Contributor

hayd commented Jan 21, 2019

@ry Did you have examples other than http? Which would be addressed with mv http/http.ts http/mod.ts.

Honestly, I don't understand the hate for explicitly defining the api in a potentially separate file (and in most cases there is only one file anyway). Edit: a la python's __init__.py.

I know I said this before: but why util? (Perhaps everything should just be in util 🙄 )
There's plenty of prior art of other languages grouping std lib modules...

@keroxp
Copy link
Contributor

keroxp commented Jan 21, 2019

I think current usage of mod.ts on the project is not consistent. mod.ts are placed on each subdirectories (I have no idea to describe those subdirectories... module set?) but its contents and structures are not equal.

//ws/mod.ts is complete implementation of ws module but //http/mod.ts is just export rotation. //http/file_server.ts has no exported member (it's just executable). //io doesn't have mod.ts, //fs/path has but //fs doesn't.

colors and flags modules are great but relatively small to compose a module set IMO (or should every single file be considered as a module set? That is why there are a lot ofmod.ts...)

@hayd I don't hate the idea itself to collect public apis into a single file. It is useful for other platforms.

But one reason why I hesitate to introduce mod.ts is related to #116. Basically, less codes are better for runtime. Collect other module's exportation into mod.ts may cause enlargement of bundled file and resolution time on browser. (that is why separated lodash module was published).

Loading mod.ts with a lot of peer exportation will be slow and resolver may go far away from origin.

Additionally, even though export rotation is useful for import, It potentially disturbs jumping to source codes in IDE. Especially in Go, It is easy to jump declaration and implementation because there are no duplicated exportation. I believe that is one of the powerful feature in typed language..

In my opinion, one of the great concept of deno was defining module as a file (and URL). I want respect its concept and designing module naming and placement of standard module is very important.

@hayd
Copy link
Contributor

hayd commented Jan 21, 2019

That's because std is a collection, they're organised in that way as each directory would ordinarily be a distinct repo (and correspond to a single name in the registry). Other projects won't have this issue: they'll only be one mod.ts.

There's a pr to export file_server, imo the actual executable belongs in examples. Agree there is inconsistency ATM, but I don't think it's fatal (or that the above solutions solve this).

I haven't tried jump to definition in VS Code , will be interesting to know if it's fooled by re-exports and/or if that's fixable.

URLs are great, structure and convention is also valuable.

@piscisaureus
Copy link
Member

Without suggesting one thing or the other on how deno_std should be organized, but semantically, I think it'd be nice to follow what rust does w.r.t mod.rs.

@keroxp keroxp mentioned this issue Feb 19, 2019
@zekth
Copy link
Contributor

zekth commented Mar 4, 2019

In addition to this there is no definition in the style guide for test naming.
For example:

  • javascript (often) foo.spec.js
  • golang foo_test.go

I can see the use in deno_std is based on the Go syntax. It might be good to put this in the style guide no? (like a recommended usage)

@zekth
Copy link
Contributor

zekth commented Mar 7, 2019

A contributing.md with the style guide would be nice to add to the repo. Your thoughts?

@bartlomieju
Copy link
Member

@zekth link is in the README: https://deno.land/style_guide.html

@zekth
Copy link
Contributor

zekth commented Mar 7, 2019

@bartlomieju So about the test syntax it is revelant to add in the style guide of deno itself? Like In deno_std we use the golang syntax for test files and give examples.

@ry
Copy link
Member Author

ry commented Mar 7, 2019

@zekth Sure you can add that to the style guide, but don't reference golang (the style doesn't originate there - it's google style AFAIK). Something like:

Unit tests should live in a sibling file to the module being tested. If the module is foo.ts then the test module should be named foo_test.ts

@bartlomieju
Copy link
Member

@ry do you still want tu pursue this issue?

@ry
Copy link
Member Author

ry commented Jun 16, 2019

No - closing

@ry ry closed this as completed Jun 16, 2019
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

No branches or pull requests

8 participants