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.

Sign up for GitHub

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

[WIP] Remote Image #67

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[WIP] Remote Image #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f525cc
Add httpkit
czystyl Feb 9, 2019
6c390f8
Add lgmp path to configuration
czystyl Feb 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions esy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,22 @@
"devDependencies": {
"@opam/merlin": "^3.2.2",
"@opam/merlin-lsp": "*",
"ocaml": "~4.7.1000"
"ocaml": "~4.7.1000",
"@opam/httpkit": "*",
Copy link
Member

Choose a reason for hiding this comment

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

This is a wrong place to add dependencies.

  1. It's a monorepo config, so anything that depends on brisk-macos will fail to find those dependencies
  2. It's devDependencies - meaning they are either for tooling like merlin, or some local deps you'd use for tests, they will not be available in runtime

We'll have a detailed design doc and contributing guide, but for now:

  1. Add those deps to the brisk-macos
  2. Also add the resolutions to the relevant example apps (we have just components for now)

"@opam/httpkit-lwt": "*",
"@opam/logs": "*",
"@opam/fmt": "*",
"@opam/yojson": "*"
czystyl marked this conversation as resolved.
Show resolved Hide resolved
},
"resolutions": {
"@brisk/core": "link-dev:./core",
"@brisk/macos": "link-dev:./renderer-macos",
"@opam/dune": "ocaml/dune:dune.opam#01b46a3",
"@opam/merlin-lsp": "ocaml/merlin:merlin-lsp.opam#3e34bb5",
"brisk-reconciler": "briskml/brisk-reconciler#fa605f1"
"brisk-reconciler": "briskml/brisk-reconciler#fa605f1",
"@opam/httpkit": "ostera/httpkit:httpkit.opam#322ca26",
Copy link
Member

Choose a reason for hiding this comment

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

Whatever you add to the resolutions, try to keep order alphabetical (scopes go first).

Copy link
Member

@wokalski wokalski Feb 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm opposed to this rule. Any rule you cannot automate is just mental overhead!

EDIT: And it's all about how you can justify that mental overhead.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to sort them by significance but that's what would be a mental overhead, so alphabetical seems like a good compromise. (most package managers do it automatically, maybe one day we'll get that for esy)

It's not like we'll add dependencies on every single PR.

Copy link
Member

@wokalski wokalski Feb 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm still opposed to it - I'll never remember/care about such artificial rule.

EDIT: I might even switch order on purpose 😂
EDIT2: re: "by significance". I could come up with an equally artificial rule - add the new one always on the bottom so that we know the order of new dependencies! And it would give as much value (i.e. none)

Copy link
Member

Choose a reason for hiding this comment

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

Aren't there enough artificial rules in this world 😄 ?

Copy link
Member

Choose a reason for hiding this comment

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

nerds

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's postpone the manual enforcing. We'll come up with a way to automate it.

Copy link
Author

Choose a reason for hiding this comment

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

Don't fight guys :D Ok, what's about to keep the opam packages at the top and the rest of packages alphabetical?

Copy link
Member

Choose a reason for hiding this comment

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

That’s what I’d usually do, but then there are scoped npm packages that mess up this logic.

Copy link
Member

Choose a reason for hiding this comment

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

Also, esy might eventually switch to scoping all npm packages with @npm/, same as with @opam/.

"@opam/httpkit-lwt": "ostera/httpkit:httpkit-lwt.opam#322ca26",
"@opam/httpaf": "anmonteiro/httpaf:httpaf.opam#57e9dd2",
"@opam/httpaf-lwt": "anmonteiro/httpaf:httpaf-lwt.opam#57e9dd2"
}
}
Loading