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

proposal: support a comprehensive meta-data file #3179

Closed
kitsonk opened this issue Oct 21, 2019 · 47 comments
Closed

proposal: support a comprehensive meta-data file #3179

kitsonk opened this issue Oct 21, 2019 · 47 comments
Labels
cli related to cli/ dir suggestion suggestions for new features (yet to be agreed)

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Oct 21, 2019

There is discussion around "lockfiles" and sub-resource integrity (see: #200).

There are also other "external" data that might be needed to describe a workload in Deno, like what the main module is, a custom TypeScript configuration, permissions and other command line options, import-maps, etc. While Deno should always just work without a meta data file, there are lots of use cases where external meta data is necessary or desired.

Providing a single file way of describing that meta data would be a desirable feature.

I propose the following:

  • Deno supports a single file meta-data schema that fully describes a workload.
  • That the format of this file would be .json
  • That when a command resolves an argument to a .json file, it assumes that it adheres to the meta-data schema, meaning that deno run app.json would read the meta data file and utilise that to determine how to cache and execute the app. This would also mean something like deno run https://example.com/app.json would also just work.

As this proposal evolves, the definition of the schema would be detailed, but would include all the functional areas mentioned above. Specifically for the "lockfile" mechanism, if an application is loaded and the .json file is writable (local) and there is no cache of the hashes of the modules, then Deno will write out the hashes of all dependent modules to the file. Upon subsequent invocations where the cache of the hashes is present, Deno will validate that all modules loaded match that cache, and if they don't they will throw and the workload would terminate.

Support a command like deno write main.ts app.json would scaffold out the meta data file, but not run the workload. Any arguments passed on the CLI could be automatically written to the file as well.

@7040934

This comment has been minimized.

1 similar comment
@7040934

This comment has been minimized.

@7040934

This comment has been minimized.

@rsp
Copy link
Contributor

rsp commented Nov 4, 2019

Any arguments passed on the CLI could be automatically written to the file as well.

@kitsonk Do you mean the --allow-* arguments here as well?

I'm wondering how the permission system would work when running, e.g.:

deno run https://example.com/app.json

with app.json having been generated with:

deno write --allow-all main.ts app.json

More specifically, what would this mean:

deno write --allow-read=X --allow-read=Y main.ts app.json
deno run --allow-read=Y --allow-read=Z app.json

Would it:

  • run with read access to X and Y
  • run with read access to Y and Z
  • run with read access to X, Y and Z (sum)
  • run with read access to only Y (intersection)
  • throw a runtime exception on access to X
  • exit with error on startup

Edit - other options that come to mind:

  • require running app.json with explicit --allow-* arguments that are the same as those required by app.json file
  • require running app.json with explicit --allow-* arguments that are compatible (a superset) of those required by app.json file
  • add --allow-meta argument to use privileges defined in the meta-data json file (could lead to problems if people get used to do it all the time)

@aliabolhassani
Copy link

Some other use cases:

  • Putting icon URI or encoded base64 data for the GUI apps Deno Standard GUI #3234
  • Device compatibility: control who runs the app

@brandonkal
Copy link
Contributor

brandonkal commented Feb 8, 2020

The issue with this is that directories will end up looking like this:

README.md
args.ts
args.json
dedent.test.ts
dedent.test.json
dedent.ts
dedent.json
doc-gen.ts
doc-gen.json
go.ts
go.json
http-cache-semantics.ts
http-cache-semantics.json
kite
kite.ts
kite.json
kubernetes
kubernetes.ts
kubernetes.json
kx.ts
kx.json
merge.ts
merge.json
promise-object.ts
promise-object.json
proxymise.ts
proxymise.json
quokka-shim.ts
quokka-shim.json
resolve-promise-object.ts
resolve-promise-object.json
runtypes.ts
runtypes.json
sourcemap-codec.ts
sourcemap-codec.json
tsconfig.json
unraw.ts
unraw.json
utils.ts
utils.json
yaml-formatter.ts
yaml-formatter.json
yaml-tag.ts
yaml-tag.json
yaml.test.ts
yaml.test.json
yarn.lock

As there isn't much info that would be provided in such a file, I'd like to propose that this information is instead embedded in the .ts file itself as suggested here: #3675 (comment)
My other issue is that it is intuitively odd to execute a json file.

@aliabolhassani
Copy link

@brandonkal I agree.

For the private artifactories, we definitely need a separate file for authentication and config purposes like .npmrc file.

Also, repeating the remote module URI in every single file is against DRY principle. So, I propose to have a different file to assign an alias name to each remote module. This way, we can have different implementation of each functionality as well. For instance:

import { select } from "sql";

instead of

import { select } from "https://deno.land/[email protected]/db/sql.ts";

Having this, modules can easily get replaced with the compatible ones just by keeping the alias name and changing the URI in the manifest file. (Like a dependency injection)

We need to have a .ts file as an entry point of the app which can re-export other config/modules like, a file for artifactory registry and another for the alias names. For instance:

mod.ts

export { registry } from ‘./manifests/registry';
export { alias } from ‘./manifests/alias’;

@brandonkal
Copy link
Contributor

@aliabolhassani You agree but then you suggest using a manifest file. I don't understand. What you describe is already possible with a importmap.

@aliabolhassani
Copy link

@brandonkal The top module, exports the manifests to Deno. Since any application only needs one configuration like permission and artifactory, no need to have manifest files for every single .ts file. This seems to be a clean way to declare manifests, however, need for the reserved words like 'registry', 'alias', 'permission', ... could be criticized.

Although, your suggestion in #3675, seems to be smart, but presenting configs as comments looks odd.

An alternate way would be to behave configs using Triple-Slash Directives in TypeScript, like:

/// <reference path="..." />
/// <permission path="..." />
/// <registry path="..." />

@KSXGitHub
Copy link
Contributor

Is this metadata file supposed to be written by human? If it is, then I suggest Deno should support formats other than JSON file.

@mathiasrw
Copy link

mathiasrw commented Mar 25, 2020

@KSXGitHub I agree. The JSON regime has been a pain for so many.

I would strongly advocate supporting the hjson file format. Relaxed syntax, fewer mistakes, more comments. See http://hjson.org. there is a rust implementation at https://crates.io/crates/serde-hjson

An alternative is json5 from http://json5.org (https://crates.io/crates/json5) or even a .yml file (even if the indent things is a bit error-prone in my opinion)

I guess it really depends on the speed of each implementation.

@KSXGitHub
Copy link
Contributor

KSXGitHub commented Mar 25, 2020

@mathiasrw HJSON is not as complicated as YAML while supporting proper string indentation. Good choice. 👍

@hayd
Copy link
Contributor

hayd commented Mar 25, 2020

toml is another obvious candidate.

IMO the way forward here is for someone to make a proof of concept as a deno installable script/separate repo, that way it could be battle tested prior to anything being included in mainline deno (I don't think we're close to that yet).

@brandonkal
Copy link
Contributor

brandonkal commented Mar 25, 2020

I'm not a fan of this proposal for reasons stated above. This should really be implemented in userland. However if Deno implements this it should be JSON. HJSON doesn't really add much value.

Write the config in whatever language you want and have it compile down to JSON before feeding it to the system.

@KSXGitHub
Copy link
Contributor

KSXGitHub commented Mar 25, 2020

@hayd I'm not a big fan of TOML. It is quite obvious that this metadata file would have nested object, which is TOML weakness. Aside from that, indentation of multi-line string is not properly handled like in YAML and HJSON.

@KSXGitHub
Copy link
Contributor

Write the config in whatever language you want and have it compile down to JSON before feeding it to the system

I disagree. Deno supports TypeScript to avoid intermediate compilation steps.

@andyfleming
Copy link
Contributor

While TOML is preferable for some people, I believe there's still a lot of javascript users that aren't familiar with the syntax.

I think JSON5 would be a great option. It's more flexible and robust than JSON while still remaining intuitive for javascript users.

@brandonkal
Copy link
Contributor

brandonkal commented Mar 25, 2020

I disagree. Deno supports TypeScript to avoid intermediate compilation steps.

So write it in TypeScript...
It handles JSON output perfectly.
Users really don't want code that requires configuration to run.

@mathiasrw
Copy link

mathiasrw commented Mar 25, 2020

@KSXGitHub Good point regarding TOML. I must say I also like the idea that you can cenerate the file as regular .json, save it and it just works.

@brandonkal I disagree. A config file like this will act like the package.json does for node - it will be hand edited often and removing dumb problems for users must be a goal for a new ecosystem. Why limit people from commenting in the config file? Why refusing to run because you added an extra , at the end of the last element? Dare I compare it to why they moved from assembly to C? It does the same but it's easier for the developer not to mess things up.

Valid json is both valid hjson and json5

@hayd
Copy link
Contributor

hayd commented Mar 25, 2020

IMO the way forward here is for someone to make a proof of concept as a deno installable script/separate repo, that way it could be battle tested prior to anything being included in mainline deno (I don't think we're close to that yet).

This first and then we can bike shed about which exts/syntax to support...

@brandonkal
Copy link
Contributor

The first step would be to consider what such a file would contain:

  1. URL to an import map (which must be JSON)
  2. URL to a tsconfig.json
  3. A string of command line arguments for deno run

That about sums it up. That can all be contained in a 3-line comment at the top of my application code.
No need for another file or a different configuration language.

So again, there is no value in duplicating package.json here. We don't need that.

@KSXGitHub
Copy link
Contributor

KSXGitHub commented Mar 25, 2020

@brandonkal That is short-sighted.

Config file is not for only Deno to read, but also for all kind of tools in the ecosystem to work with. Just like in Node.js, package.json can be read by npm, webpack, jest, etc. Encode them as comments in JavaScript/TypeScript file would force all these tools to use an AST parser.

Config file is not read-only either, it can be modified by machine. Embed them as comments in code would lead to all kinds of problem, such as code style not being preserved.

Furthermore, comments were never meant to be read. Using comments as config would be clumsy.

there is no value in duplicating package.json here

So you duplicate package.json 3 times instead? (importmap, tsconfig.json, deno run arguments).

@brandonkal
Copy link
Contributor

Other tools have their own config files. Deno doesn't need a config file.

You can try to have 10-20 tools reading and writing to the same config file, but that isn't scalable. It also encourages copy-and-paste. You are better off linking to your tsconfig.json which is required for other tools such as VSCode.

package.json isn't scalable. It's the reason we have a seperate tsconfig.json. I understand you are familiar with package.json, but it is not something you should look to model.
Importmap is a web standard, so it gets its own file.

So that just leaves run arguments. It is trivial to write a tool that parses JSDoc and extracts Deno run arguments. Do you really want to litter your projects with yet another file, deno-config.json to document some deno run arguments? I won't be doing that. There's a lot of value in having documentation with the code. It's why we use tools such as JSDoc, rustdoc, godoc, etc. Why not also document which run arguments are required to execute that file with the rest of your module's documentation?

@KSXGitHub
Copy link
Contributor

It is trivial to write a tool that parses JSDoc and extracts Deno run arguments

Please don't tell me you would use RegExp.

@KSXGitHub
Copy link
Contributor

Also, your suggestion would lock user to a particular file type: JavaScript/TypeScript while Deno can also run WASM.

@brandonkal
Copy link
Contributor

brandonkal commented Mar 25, 2020

Nice try. I don't have anything more to add here. My point still stands: #3179 (comment)

  • There are three strings that such a metadata file would contain.
  • I've shown a package.json clone does not scale
    Three values does not warrant its own file. Parse it however you like.

Also, placing metadata in the entrypoint module does not lock people out of using WASM.

@stale
Copy link

stale bot commented Jan 6, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added stale and removed stale labels Jan 6, 2021
@kitsonk kitsonk added cli related to cli/ dir suggestion suggestions for new features (yet to be agreed) labels Jan 7, 2021
@bartlomieju bartlomieju mentioned this issue Jun 28, 2021
23 tasks
@ry
Copy link
Member

ry commented Aug 12, 2021

Goals:

  • too many flags
  • have lock file by default
  • define a "project structure" for deno projects, maybe even introduce a "deno new" command that could generate
  • give LSP a clue if project is Deno or not
  • portable across platforms
  • config file should be safe to check into git
  • Deno should be able to detect config file automatically
  • works with Deno Deploy

Non-Goals:

  • general purpose scripting language
  • package.json 2.0
  • "lib manifest"
  • don't assume libraries have config file
  • don't allow for config file to be a place where random projects drop unrelated configuration (e.g. how vscode uses package.json)
  • don't want to have options that are not flags
  • don't store personalized config (like auth tokens)
  • don't store package metadata like version (which should be done with git tags instead) license, description, name.

@crowlKats
Copy link
Member

define a "project structure" for deno projects, maybe even introduce a "deno new" command that could generate

Deno should be able to detect config file automatically

I am against these. The former sounds like it will "break the current freedom and flexibility", as people can structure their projects however they want. Even if this is just a guideline and not a rule, people will feel coerced to use it over what they want, especially as the CLI itself is suggesting the structure. Also this will make people request a lot for an entry point, and at that point we might as well be package.json.
For the latter, this starts Deno into magic resolution, which is something that has being fought against, and in my opinion, correctly so. just having a config flag (yes i am aware this would collide with ts config, just an example) with a short version would be easy enough and realistic for usage without being too cumbersome: -c config.json.

We should add a generate subcommand that generates a meta-data file from given flags

@ry
Copy link
Member

ry commented Aug 13, 2021

We want to take a step in the direction of a config file, but there is a lot of controversy. Therefore proposing the following minimal possible config:

  1. Deno will lookup a file called Deno.json in the current working directory on startup.
  2. This Deno.json can also be specified by the already existing --config flag
  3. Deno.json can have a top-level "compilerOptions" for configuring typescript.
  4. We will initially support one other top-level configuration called "lint" which will allow people to configure how deno lint works. Example:
{
  "compilerOptions": {
    "jsxFactory": "h"
  },

  "lint": {
    "ignore": ["testdata/"],
    "rules": {
      "tags": ["recommended"],
      "include": [
        "ban-untagged-todo"
      ]
    }
  }
}

@grian32
Copy link
Contributor

grian32 commented Aug 13, 2021

@ry That would be a good minimal first pass, although I'd like to propose having a permissions object(that would look something like below), that would allow specifiying permissions for the run, test and other commands that may take permission flags.

{
  "compilerOptions": {
    "jsxFactory": "h"
  },

  "lint": {
    "ignore": ["testdata/"],
    "rules": {
      "tags": ["recommended"],
      "include": [
        "ban-untagged-todo"
      ]
    }
  },
  
  "permissions": {
    // it can take an allow list or a boolean, boolean is just to enable it for everything, allow list is self explanatory
    "allow-run": "allow-list-goes-here",
    "allow-read": true,
  }
}

This would be helpful as trying to run with as strict permissions as possible can make a command get pretty long, i.e

deno run --allow-run="./testfiles/*" --allow-net="localhost:4044/api/*" --allow-hrtime --allow-plugins --allow-read="./logs/*" 

@caspervonb
Copy link
Contributor

caspervonb commented Aug 13, 2021

Some thoughts on the automatic-resolution.

  1. Deno will lookup a file called Deno.json in the current working directory on startup.

I get this is probably coming from an "it should be easy to use" point of view, which is an understandable concern but I also think this has the potential to become problematic down the line.

  • Obviously you know this but no magic resolution was an expectation made early on. Not really an argument for or against bit it just feels off to add magic resolution now. If we look to browsers for prior art, manifests do exists but they need to be explicitly opted in to with tags.

  • Permissions will get requested a lot, presumably added early on and with that permissions are longer explicit. Passing a configuration file in with say -c config.json is at-least an explicit user action.

  • This last one is a bit of a stretch from this pass but at some point import-maps would have to make it into the configuration file and with that there's plenty of room for user failure and fracturing the ecosystem (Thinking along the lines of "you need to use this Deno.json file to use my library which uses bare identifiers"). Resolving it to the current working directory mitigates this to some degree and makes it more of an authoring tool. But the distance from here to "modern" package.json seems a bit slippery and filled with unknowns.

@ry
Copy link
Member

ry commented Aug 13, 2021

I'd like to propose having a permissions object

@grian32 We're interested in solving this problem too, but we can't come to an agreement yet about how this would look. For example, what if you had two different programs frontend/main.ts and backend/main.ts that you'd like to have different permissions for. We'll punt on deciding how that looks until we get this initial bit in place.

@ry
Copy link
Member

ry commented Aug 13, 2021

Obviously you know this but no magic resolution was an expectation made early on. Not really an argument for or against bit it just feels off to add magic resolution now.

@caspervonb There's no change to the module resolution in the MVP above. I agree about the permissions - but there's still some controversy about how they should be specified (especially when a project contains multiple entry points) - so I'd prefer to punt on that now and just nail down what we can agree on.

@grian32
Copy link
Contributor

grian32 commented Aug 13, 2021

I'd like to propose having a permissions object

@grian32 We're interested in solving this problem too, but we can't come to an agreement yet about how this would look. For example, what if you had two different programs frontend/main.ts and backend/main.ts that you'd like to have different permissions for. We'll punt on deciding how that looks until we get this initial bit in place.

Alright sounds good! On the different entrypoints, you could always just have two different Deno.json for each.

@bartlomieju
Copy link
Member

I'd like to propose having a permissions object

@grian32 We're interested in solving this problem too, but we can't come to an agreement yet about how this would look. For example, what if you had two different programs frontend/main.ts and backend/main.ts that you'd like to have different permissions for. We'll punt on deciding how that looks until we get this initial bit in place.

Alright sounds good! On the different entrypoints, you could always just have two different Deno.json for each.

Not really, because it leads to a situation described in: #3179 (comment) and then you'd have to configure lint config on both of them?

@sylc
Copy link
Contributor

sylc commented Aug 13, 2021

called Deno.json

Beeing able to write comments in this file would be great. (so not a strict json I hope)

@bartlomieju
Copy link
Member

called Deno.json

Beeing able to write comments in this file would be great. (so not a strict json I hope)

It will be strict, but you'll be able to use Deno.jsonc as well

@caspervonb
Copy link
Contributor

There's no change to the module resolution in the MVP above. I agree about the permissions - but there's still some controversy about how they should be specified (especially when a project contains multiple entry points) - so I'd prefer to punt on that now and just nail down what we can agree on. - @ry

Oops, bad choice of words to use resolution in this context. I just meant "magically resolving a file" in general which in this case the default configuration file.

Fair enough on punting 👍

Not really, because it leads to a situation described in: #3179 (comment) and then you'd have to configure lint config on both of them? - @bartlomieju

Just throwing it out there, one option that is familiar to many is to do tsc style composable configurations.

@brandonkal
Copy link
Contributor

brandonkal commented Aug 13, 2021

  1. Deno will lookup a file called Deno.json in the current working directory on startup.

Please reconsider resolving implicit dependencies. If a file is not in the program’s input chain or cli call it should not be automatically pulled in. It will break a lot of Deno’s original promises for little benefit. If you look at the majority of programs, input comes from command-line arguments, environment variables and specific absolute config filepaths. Build tools are the exception but deno run is not that.

I’ve found Deno to be most useful as a general scripting runtime. It’s enabled me to write one-off scripts and also build CI systems for non-JavaScript projects. A lot of power in a single TypeScript file. A lot less portable if I have to worry about a json file in any given directory. That’s just one example of a workflow that breaks if a file is automatically pulled into context. A breaking change.

  1. This Deno.json can also be specified by the already existing --config flag

This makes sense.

@shalvah
Copy link

shalvah commented Sep 8, 2021

I'd like to propose having a permissions object

Isn't there something of an attack vector here: a program run with write permissions could edit the Deno.json file and give itself higher permissions (for next time)? Perhaps the Deno.json file will only permit read requests.

@crowlKats
Copy link
Member

a program run with write permissions could edit the Deno.json file and give itself higher permissions

write permissions can be granular; this would be in the user's hands to give the permissions they intend and want to give.

@nayeemrmn
Copy link
Collaborator

It is definitely attack surface area though. Deno.json unlike the Deno executable will usually be in the current directory and dodging it for permissions would require a significantly more verbose command. I agree on punting at least, let on-disk permissions be the last thing we address.

@grian32
Copy link
Contributor

grian32 commented Sep 8, 2021

I was generally thinking of dev experience and the fact that I just end up using -A a lot because these commands get pretty wordy so I'm definetly giving code more permissions than it should have. I'm not sure about the security aspect, just thought I'd give some updated reasonings.

@crowlKats
Copy link
Member

The main goal of having a single config file has been achieved, and ideas like providing permissions in it have their own issues (like #12763).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

No branches or pull requests