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

Reorg #76

Closed
ry opened this issue Jan 3, 2019 · 23 comments · Fixed by #105
Closed

Reorg #76

ry opened this issue Jan 3, 2019 · 23 comments · Fixed by #105

Comments

@ry
Copy link
Member

ry commented Jan 3, 2019

We've been adding code without much thought to the overall directory structure. I suggest the following renames:

//net/http.ts -> //http/mod.ts
//net/file_server.ts -> //http/file_server.ts

//net/bufio.ts -> //io/bufio.ts

//net/textproto.ts -> //textproto/mod.ts

//path/index.ts -> //fs/path.ts
//mkdirp/index.ts -> //fs/mkdirp.ts

//logging/index.ts -> //log/mod.ts

//flags/index.ts -> //flags/mod.ts

//colors/index.ts -> //colors/mod.ts

Along with the associated tests.

If you're worried about breaking existing code:

  1. People should link to tagged releases of deno_std ... EG "https://deno.land/x/std@v0.2.4/net/http.ts" .. we do not make stability guarantees for master.
  2. It's better to make these changes now, when deno_std is young than later.
@hayd
Copy link
Contributor

hayd commented Jan 3, 2019

Very much approve of this issue/cleanup but

Not a huge fan of "utils". What is the logic for putting datetime there? What else is a util?

@ry
Copy link
Member Author

ry commented Jan 3, 2019

//datetime/mod.ts -> //utils/datetime.ts

datetime is very small... seems to not deserve its own directory. I agree it seems odd that its the only "util". I will remove it from the above list. Maybe we can do that later when there are more utilities.

@ry
Copy link
Member Author

ry commented Jan 3, 2019

//net/bufio.ts -> //io/bufio.ts
//net/textproto.ts -> //textproto/mod.ts

I've also updated the above list with these suggestions.

@keroxp
Copy link
Contributor

keroxp commented Jan 5, 2019

Please let me express the opposite opinion. I understand we need some consistent style or rule about module file's naming. However, in my opinion, mod.ts can't be consistent because there can be directories with no mod.ts (e.g. io directory may not have mod.ts because there are relatively many separated files). Some developer may get confused no mod.ts exists.
If you plan to place mod.ts because of having less files in directory than others, I think it is better to place it on root directory with directory's name (//textproto/mod.ts -> //textproto.ts).
One another reason I counter is about test files . To follow style of Golang, It is easy to understand that codes and tests are flatly placed by same naming rule (adding _test postfix).
I worry about it beeing the strict standard of module file naming in deno after 10 years.

@kitsonk
Copy link
Contributor

kitsonk commented Jan 5, 2019

I believe a collection of modules, something we would put in std in a sub directory, should have a module named mod.ts, even if all it does is export the "public" API surface that is intended for that collection. There needs to be a "default" place to go look. Even if it is a loose collection of functions and classes broken up across modules, the then they should all be re-exported from mod.ts.

@keroxp
Copy link
Contributor

keroxp commented Jan 5, 2019

Thank you for the opinion!

From style guid, I understand mod.ts was ported from Rust's unofficial module convention. I doesn't know much about Rust, so looked around some famous rust codes on Github.

Even in rust community, mod.rs (or lib.rs?) doesn't looks predominant idea for representing module entry file, for me. For example, ring, the famous crypto library, has decided to stop using mod.rs just 27 days ago (briansmith/ring@69cc075) because
mod.rs might not be sensible (commented).

As @kitsonk says, Some developer may need "default" entry file for modules. But Go doesn't have, but succeeding because of a type and module system designed based on URL-like. Thanks to the modern intelligent editor, importing classes, functions and constants is very easy today. The only thing developer must know is component name not entry file's url.

In my opinion, why the entry file that is composed of all internally exposed component from other files was needed, is classic programming languages couldn't import module directly from URL! (like ruby, python and npm do)

I believe deno's module system based on URL is pretty nice. If mod.ts were implicitly required, It means that mod.ts can be omitted.

On the other hand, I understand that It is difficult to create "package-private" (not desirable to be public) components in deno's module system. But that is ES/TS Module' problem. There must be another solution. Thanks!

@ry
Copy link
Member Author

ry commented Jan 5, 2019

@keroxp makes a good point. I suggested mod.ts as an alternative to using index.ts - but a more minimal solution would be to simply suggest people to avoid index.ts in the style guide.

@hayd
Copy link
Contributor

hayd commented Jan 5, 2019

Ring/that issue has a history, see briansmith/ring#598 etc.
Anyway this is just a style guide not rejecting certain configurations (as rust were).

We're also not talking about a mod.ts for each file which is what ring maintainer took exception to with mod.rs. In deno you can use relative imports trivially within a project.

ring still uses lib.rs which is what this is more analogous to imo. If there are several files a mod.ts exposing all of them is a reasonable and offers standard entry point to a public API for the repo.

I'm very much in favor of this consistency (at least within std)

@keroxp
Copy link
Contributor

keroxp commented Jan 6, 2019

Sure, I let it decided totally by @ry
But may I ask you more questions?

In deno_std,

  • Should every sub(and sub)directories have mod.ts for default entry point?
  • Should there be no module files on the root directory?
  • Collecting public api to mod.ts means there are duplicated exportation on the project, is it desirable?
    • deno resolves "Aclass" from https://deno.land/x/some/mod.ts,
    • mod.ts exports "Aclass" from "../aclass.ts"
    • deno downloads modules recursively, both mod.ts and aclass.ts will be downloaded.
    • As a result, there are two identical exportation about Aclass on ~/.deno/deps/https/deno.land/x/some
    • Editor will show duplicate suggestions before importing because both are valid importing reference.
    • 2019-01-06 18 39 57
    • Ideally, contentType in mime.ts should be package-private in above case.
    • If can be, mod.ts is considered as consistent....
    • Go does it by initial-based ACL 😩
  • Frankly, I think it is not good idea to collect public api to one file, especially deno's module system even though it was good idea on other languages.

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Jan 6, 2019

The duplicate suggestions part sounds more like a problem with the editor itself. It might be possible that the editor could prefer exports from mod.ts over those from other files (a simple reordering of suggestions sounds good enough for many use cases). (definitely not a good idea, but the editor could also possibly choose to ignore/gray out exports from file with names, say, starting with _, when a mod.ts is present in the same folder (when the user is not editing files inside that folder))

As to mod.ts for subdirectories, I don't think it would be that necessary unless we want to have public submodules. I deem mod.ts more or less serve just as a convenient entry point for users and a convention that could be changed if general feedback is against such design.

Also, I am somehow against package private design implemented by Deno but not present in ES/TS standards/proposals with positive feedbacks (opposite to things like globalThis which, as unhappy as I am with the naming, Deno should support it) Probably we could resort to namespaces at times?

@kitsonk
Copy link
Contributor

kitsonk commented Jan 6, 2019

It might be possible that the editor could prefer exports from mod.ts over those from other files (a simple reordering of suggestions sounds good enough for many use cases).

I believe this is something that could be added to deno_ls_plugin fairly easily. I think re-ordering (versus eliminating) would be best.

As to mod.ts for subdirectories, I don't think it would be that necessary unless we want to have public submodules. I deem mod.ts more or less serve just as a convenient entry point for users and a convention that could be changed if general feedback is against such design.

I agree with this wholeheartedly.

@keroxp
Copy link
Contributor

keroxp commented Jan 7, 2019

@kevinkassimo

The duplicate suggestions part sounds more like a problem with the editor itself.

I disagree with it. It is difficult to judge which file are prior exportation about it without any "rule". There must be some configurations about module resolution priority for the editor.

Now I agree It is convenient that there are files somehow collect api, so could you let me suggest another reorg style on this repository?

As @ry said, if mod.ts were proposed simply instead of index.ts, there can be other styles.
For example, I prefer styles like this:

  • //logging.ts
  • //logging_test.ts
    or
  • //logging/logging.ts
  • //logging/logging_test.ts

I think It is not desirable there are many mod.ts files on the same project, especially managed in monorepo style.

And I am also impressed that mod.ts can be another index.js on node.js. If mod.ts become to be "standard" entryfile, we may allow beeing of server which redirects shorthanded (without mod.ts) request into ./mod.ts. Is it available or acceptable on deno's philosophy? (Current version's deno executable accepts URL without ".ts.)

For example, if there could be importing like this:

  • import * as http from "https://unpack.land/x/http"

then unpack.land may try to resolve x/http.ts, x/http/mod.ts, x/index.ts and x/http/http.ts in order like browser. It is useful for someone or not for another one, but anyway must be ambiguous.

Also, I am somehow against package private design implemented by Deno but not present in ES/TS standards/proposals with positive feedbacks

I agree with it. I also think that It is desirable there are no deno's specific module rule against ES/TS module.

@kevinkassimo
Copy link
Contributor

@keroxp fyi extensionless support has been dropped in denoland/deno#1396 . As for other part of your argument, please give me more time to think about and I’ll reply later...

@keroxp
Copy link
Contributor

keroxp commented Jan 10, 2019

@kevinkassimo There seems to be unmerged reorg PR with mod.ts styles. and third party's libraries are starting to follow this style. So I drop my proposal for speeding up. Keep going.

@kevinkassimo
Copy link
Contributor

@keroxp I’m still collecting opinions on mod.ts from others I know. Since it is currently still at an early stage of adoption it should be easy to change if given more negative feedbacks

@tunnckoCore
Copy link

tunnckoCore commented Jan 11, 2019

Hm, sorry but trying to keep up. Can we read somewhere about the mod.ts decision?
I don't see why /index.ts is not an option? If it's only because "it's shorter", that's not an argument to me, and definitely, don't like it. Why not think upfront for smooth transitioning by node.js folks or newcomers? Why should follow conventions or ideas from other languages such as Rust and Go, when we already have mostly 10 years of experience with Node.js and enough conventions and understandings?

Why not main.ts? This at least follows at some point more of the other languages. And it's a good word, making sense.

As for the automatic resolving,

For example, if there could be importing like this:

I'm agree with @keroxp. In same time, I understand that it defeats the idea for URL based module system. Because one of the main ideas with Deno is to be as much as possible like in the browsers, except that it's in TypeScript. But anyway, we already have import * as deno from 'deno' which isn't compatible with browsers anyway, so I think it's acceptable to have such exception for automatic resolving for index.ts / mod.ts or whatever, such as in the Node.js land.

Btw, what about supporting from 'deno_std' shorthand, such we have from 'deno'?

@ry
Copy link
Member Author

ry commented Jan 12, 2019

@tunnckoCore I wrote about it in the style guide (README.md) of this project:

index.ts comes with the wrong connotations - and main.ts should be reserved for executable programs. The filename mod.ts follows Rust’s convention, is shorter than index.ts, and doesn’t come with any preconceived notions about how it might work.

@kitsonk
Copy link
Contributor

kitsonk commented Jan 12, 2019

In same time, I understand that it defeats the idea for URL based module system. Because one of the main ideas with Deno is to be as much as possible like in the browsers, except that it's in TypeScript.

It isn't the browser the resolves index.html, it is the server that does it and tells the browser it is "such and such URI". That non-explicit nature has always caused issues. So the decision was to always try to be explicit, no magic and index.html implies that there is/will be some magic.

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Jan 14, 2019

So there has been an alternative proposal that I have thought of in the past and brought up again recently in discussions with some developers (including @justjavac).

Since extensionless support has been dropped, there are no more conflicts between log.ts and log/. Thus it might be possible to adopt a directory structure like this:

- log.ts       # External API are exported here. Uses or re-exports imports from log/mod_a.ts and log/mod_b.ts
- log/         # We don't need this directory if log.ts if self-contained
  |_ mod_a.ts  # internal code
  |_ mod_b.ts  # internal code

With such directory layout, we could do

import "https://deno.land/x/std/log.ts";

instead of

import "https://deno.land/x/std/log/mod.ts";

There are some issues also with this approach.

  1. Ideally, it might be possible to import a module from registry by
import "https://deno.land/x/${module_name}.ts"

However, this requires the registry to extract ${module_name} from the url and use it to decide how forwarding is done.

  1. The directory structure might be messier with there are multiple submodules

Also cc @keroxp about this proposal

@hayd
Copy link
Contributor

hayd commented Jan 14, 2019

Is there a clever way to do this automagically, so that:

https://deno.land/x/${module_name}.ts

rendered a typescript file that imported and exported everything from

https://deno.land/x/${module_name}/mod.ts

... without having to actually run/parse mod.ts on the registry server.

@kitsonk
Copy link
Contributor

kitsonk commented Jan 14, 2019

Generally "automagically" !== good idea... every time it bites you in the 🍑, that is the main justification for staying away from index.ts... @kevinkassimo proposal isn't bad because it has no ✨🦄.

@hayd
Copy link
Contributor

hayd commented Jan 14, 2019

By "automagically" (silly word, I shouldn't have used it) I just meant populate https://deno.land/x/${module_name}.ts, nothing magical, and would be entirely within the registry.

Edit: the obvious thing to put in would be:

Import * as log from './log/mod.ts';
export { log };

You can do that in the registry Lambda...

@ry
Copy link
Member Author

ry commented Jan 14, 2019

I'm maybe open to deno.land registry hacks like that but this discussion is more about the style guide for deno in general.

  1. I think we all agree that index.ts is a bad idea because it comes with the wrong connotations.
  2. We'd like to avoid a lot of module specifiers that look like this "/foo/bar/mod.ts" - it's a waste of space when "/foo/bar.ts" would work.
  3. We'd also like to avoid a lot of module specifiers that look like "/foo/bar/bar.ts" - also a waste of space.
  4. I'm hesitant to populate the root directory of deno_std with .ts files. It seems messy.

I feel like the ideal situation would be if we figured out some top level categories like "fs" and "net" that we could organize into? E.G.

//term/colors.ts
//fs/path.ts
//net/http.ts

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 a pull request may close this issue.

6 participants