Closed alestiago closed 5 months ago
renancaraujo
commented
1 year ago I don't think we should add this.
From the CLI standpoint, all commands are supposed to be managed at a project level. What is the use case for this to justify opening an exception?
in the given example, "new route" command will be executed with herbal but the route created will be items/tea/herbal. In this context, herbal is not a route.
alestiago
commented
1 year ago From the CLI standpoint, all commands are supposed to be managed at a project level. What is the use case for this to justify opening an exception?
I think we should consider this as an exception.
The main use case is to share logic between different IDE extensions (Visual Studio Code, Android Studio, etc), see this. There is some logic in the IDE extensions associated with parsing the working directory to become the expected input for dart_frog new.
The second use case is to improve the experience of those developer's currently using the CLI. It is faster and more intuitive to change directories when using simultaneous and related dart_frog new commands than to manually adjust the route several times. Perhaps, this is why @Luckey-Elijah dropped a 👍 .
in the given example, "new route" command will be executed with herbal but the route created will be items/tea/herbal. In this context, herbal is not a route.
I don't know if I follow the statement. As far as I am concerned doing the above does throw the error as detailed in the current behaviour, see demo video.
renancaraujo
commented
1 year ago I don't know if I follow the statement.
Lemme rephrase then: If this change is implemented, in the proposed scenario, dart_frog new route "herbal" will create a new route "items/tea/herbal".
"new route herbal" -> creates new route "items/tea/herbal". It is a misleading command to say at least.
If we go forward thinking that relative routes are not only relative to other routes, but also relative to file paths (which is conceptually a mistake), we have to think about the edge cases:
dart_frog new route ../new_route on working dir routes/dart_frog new route /new_route vs dart_frog new route new_routeshare logic between different IDE extensions
On that front, we should follow the approaches of other extensions/plugins (dart & Flutter, for instance) and have it to take active actions only when a dart_frog project is open.
Even on passive actions (such as commands), that seem very particular to how the extension is implemented, see how dart/flutter finds the root project: https://github.com/Dart-Code/Dart-Code/blob/c18efb17ecfc721768b7a0aa0310535dcfd5d16b/src/extension/project.ts
... is to improve the experience of those developers currently using the CLI
This is debatable. Having commands (dev, run, and possibly move and remove in the future) to be run from the project root removes the need for people to walk about in the routes dir as well as manage those files manually in most cases.
alestiago
commented
1 year ago Thanks @renancaraujo for rephrasing. I think I'm starting to understand your POV, here are some of my thoughts:
If we go forward thinking that relative routes are not only relative to other routes, but also relative to file paths (which is conceptually a mistake), we have to think about the edge cases: dart_frog new route ../new_route on working dir routes/ dart_frog new route /new_route vs dart_frog new route new_route
I'm not asking to consider the route path as a file path. I'm asking to consider the working directory as the "prefix" of the route path if executed within a Dart Frog project routes. Therefore, I don't think this use-cases would be a problem since the current behaviour can still remain.
A separate issue can be made if we want to support specifying route paths as we specify file paths (for example, allowing ../hello/a). Currently, given the CLI and extension context, I don't see an immediate advantage besides saving to change directory.
On that front, we should follow the approaches of other extensions/plugins (dart & Flutter, for instance) and have it to take active actions only when a dart_frog project is open. Even on passive actions (such as commands), that seem very particular to how the extension is implemented, see how dart/flutter finds the root project: https://github.com/Dart-Code/Dart-Code/blob/c18efb17ecfc721768b7a0aa0310535dcfd5d16b/src/extension/project.ts
As far as this issue concerns, I don't think this is directly applicable to the Dart and Flutter extensions since they do not offer any generation from the context menu with similar requirements. Nevertheless, I'll add some details to this point. The extension currently detects the scope of a Dart Frog project by checking the closest routes directory (this is required due to the use-case explained at the bottom of this comment), unlike here where the logic is simply checking for a routes direct subdirectory.
I think it's obvious that we can improve the logic to detect the closest Dart Frog project in both the CLI and extension (for example, considering the pubspec.yaml and a dart_frog dependency). I think this is deviating from the main issue outlined and an additional issue can be opened to tackle this.
This is debatable. Having commands (dev, run, and possibly move and remove in the future) to be run from the project root removes the need for people to walk about in the routes dir as well as manage those files manually in most cases.
I agree that the developer experience can be debated. From my personal usage, I did find that considering the local working directory as the prefix for the route (if it's inside a dart frog project) improved the developer experience; since it removes friction during development. The "friction" is illustrated in the following scenario:
routes/shop/eggs/medium and you want to create routes/shop/eggs/large.dart_frog new route large but are thrown "_No routes directory found in the current directory. Make sure to run this command on a dartfrog project". As a developer you might be slightly confused since you are actually inside a Dart Frog project! However, you are simply not at the root of the Dart Frog project.dart_frog new route large you have to change directory to the root first and then run dart_frog new route shop/eggs/large to get the desired outcome.Personally, I think the scenario shows how the current behaviour introduces friction for developers using the tool.
If we look at other CLI's such as Mason CLI (although with a different feature set) it does consider the working directory to alter the output. For example, doing mason make my_awesome_brick will make the template in the working directory instead of complaining that you are not at the same level as the mason.yaml and force you to specify the --output-dir to make the template. This example is loose, but hopefully it helps painting the picture.
Although still valid, I consider the above as a second use case of this issue and not as the main use case. The main use case and motivation is explained below.
I would like to clarify and empathise the main motivation of this issue. As I detailed previously:
The main use case is to share logic between different IDE extensions (Visual Studio Code, Android Studio, etc), see this. There is some logic in the IDE extensions associated with parsing the working directory to become the expected input for dart_frog new.
Since within IDE's the user directly visualizes the entire directory within a context menu, we allow them to left click a specific directory or file. In return, the IDE API will give us the Uri where the user selected. In the case of a New Route we additionally ask for the route name, in the case of a New Middleware we do not ask for any further information. This simplifies the need of the user having to specify the route path twice since it can be deduced from the Uri they selected.
This is how the logic looks in the VS Code extension. Although we do not have an IntelliJ extension yet, the logic will be similar since, as far as I know, the Uri is retrieved in a similar manner, see this. I think, it would be great to somehow share parsing the Uri and formatting it as a valid route path; and adding a mechanism to consider a working directory within a Dart Frog project as the prefix of the route would solve this.
erickzanardo
commented
1 year ago Catching up on this discussion, I will be probably be adding more fire to discussion 😅, but here are my two cents:
As a heavy terminal user, I believe that considering the local dir would make my life in the terminal easier, as I would prefer to:
mkdir routes/bla
cd routes/bla
dart_frog new a
dart_frog new b
...Instead of
dart_frog new routes/bla/a
dart_frog new routes/bla/b
...Ideally, would be cool if the new command accepted multiple routes name, like
dart_frog new a b c d ...With that said, I think it is important to highlight some stuff
All in all, I believe that it is worth to add the proposed changes and make the new command take into account the local directory, I think it is a good improvement. But also, I don't think this is a huge improvement to the CLI user, so if there are too many complications that this could bring, we should keep it as is.
alestiago
commented
1 year ago Thanks @erickzanardo for hopping in and dropping your two cents.
Ideally, would be cool if the new command accepted multiple routes name, like
I think that is a cool idea and I believe it deserves an issue of its own.
We shouldn't design the CLI in favor of other tools IMO, in this instance the extensions, their usage are different and one shouldn't be design in pro of the other, the CLI should be designed in favor of the CLI user and although it is totally ok that the IDE extension uses the CLI, the IDE extension should adapt to the CLI and not the other way around.
I agree. My current perception was that some CLI functionality was motivated purely because of extensions. In other words, I was considering extension as users of the CLI. I kind of derived this assumption from this comment over here. Where we holded back from developing extensions until the CLI had the functionality to avoid duplication of code. However, if we think the solely users of the CLI are the developers, then I agree, we should be cautious of adding functionality to favour extensions. Regardless, in this use-case, as you well pointed out, I think an exception should be considered.
erickzanardo
commented
1 year ago After discussing more with in depth this with @renancaraujo I think we should reevaluate this.
When creating a new route via the CLI, we inform a route path, not the file path, so "working directory" is simply something that doesn't exists in this context.
I understand that right now, the file path and route path is super similar, but this is an important semantic concept that I understand that we need to keep as it is, and I also feel that it is important that the IDE extension follows this same idea, meaning that when creating a new route on the extension, you should also provide a route path, and not a file path.
alestiago
commented
1 year ago After discussing more with in depth this with @renancaraujo I think we should reevaluate this.
Besides the rest of your comment is there anything else that was discussed in this off-GitHub conversation that could be useful here? I would have loved to be part of the discussion.
When creating a new route via the CLI, we inform a route path, not the file path, so "working directory" is simply something that doesn't exists in this context.
I agree that route path and file path are conceptually different. Excluding rogue routes, whilst under routes/ is the relative file path to a route not considered its route path?
and I also feel that it is important that the IDE extension follows this same idea, meaning that when creating a new route on the extension, you should also provide a route path, and not a file path.
The extension is not asking to provide a file path. In other words, in the IDE extension you do not specify the file path but instead, the route path relative to where you selected the directory. To what extent is this violating the conceptual difference?
erickzanardo
commented
1 year ago Besides the rest of your comment is there anything else that was discussed in this off-GitHub conversation that could be useful here? I would have loved to be part of the discussion.
Not much really, he just explained to me about the difference about the file path vs route path, which was something that wasn't clear on my mind at the moment that I wrote my original comment.
I agree that route path and file path are conceptually different. Excluding rogue routes, whilst under routes/ is the relative file path to a route not considered its route path?
Like I mentioned in my previous comment, indeed right now, if you exclude the routes/ prefix, the file path and route path are basically the same (if you map/replace index and .dart ofc).
But if we change the CLI to accept a file path instead of a route, it will be a breaking (which I guess is fine since we are in pre v1 for now, but still something to consider), but we will lose some interesting features to explore in the future, like we could add the HTTP Method as an optional on the route param, something like dart_frog new "GET /bla/ble" or maybe dart_frog new "PUT|POST|DELETE /bla/ble" or maybe the methods could be parameters instead if we think it would be better, etc, and then the cli will decide if it will create a ble.dart file, or an index.dart that will handle multiple methods, etc.
The extension is not asking to provide a file path. In other words, in the IDE extension you do not specify the file path but instead, the route path relative to where you selected the directory. To what extent is this violating the conceptual difference?
Got ya, so In that case, can't the Extension have some custom code that, given a path, will resolve it to its route path, and then simply concat with the inputed relative path by the user? Then just pass it to the cli?
I also wonder if that would be possible in the CLI as well, would be an interesting addition to the CLI as well.
alestiago
commented
1 year ago Hi @erickzanardo thanks for the prompt reply.
But if we change the CLI to accept a file path instead of a route.
I'm not asking to consider the route path as a file path. I'm asking to consider the path relative to the /routes within the working directory as the "prefix" of the route path. If and only if executed within a Dart Frog project /routes directory, as mentioned here.
Got ya, so In that case, can't the Extension have some custom code that, given a path, will resolve it to its route path, and then simply concat with the inputed relative path by the user? Then just pass it to the cli?
That is how it is currently implemented.
I also wonder if that would be possible in the CLI as well, would be an interesting addition to the CLI as well.
Indeed, that is exactly what I'm requesting here to consider and possibly do.
erickzanardo
commented
1 year ago Got ya, got ya, sorry for all the confusion that we had to go in order to arrive at this point, but this can get quite confusing.
So, keeping in mind that the new command will still be receiving a route param, IMO, changing so that the command can accept an absolute and a relative route path, I think this is how it should look like:
dart_frog new /turtles/[id]Should be kept the same, it must be run from the root, and will always create the full route.
// assuming the user is at route/turtles/[id]/
dart_frog new gearsWill create a /turtles/[id]/gears
I feel that relative commands like this should have additional checkings though, like prompting the user something like:
This will create the following route: `/turtles/[id]/gear, proceed? yN(which could be overridden by a -y argument or something. This command would also break if it is ran in a folder that is not inside routes or if by some reason, the current working dir is not resolved to a route path.
I feel this is a good middle ground between Renan's concerns and Alejandro's idea, what do you all think?
cc @renancaraujo @alestiago @wolfenrain
renancaraujo
commented
1 year ago I can summarize why this is a bad idea (even with a prompt) in three main concerns on this:
The logic of concatenating paths to discover the root of a DF project is so trivial that I need help understanding the extension thing as a motivation here.
We are still determining what an extension/plugin will look like after the daemon addition. The very concept of the daemon is to manage dart frog projects, and you can only do that with the dart frog root path figured out. THERE WON'T BE ANY METHODS TO BE RUN FROM NON-DART FROG ROOT DIR PATHS or that daemon code will be unbearable (see item 3).
I brought up the code from the dart extension as an example of how it figures the path of the root of dart packages in an automated way. No prompts. No error.
I'm not asking to consider the route path as a file path. I'm asking you to consider the path relative to the /routes within the working directory as the "prefix" of the route path.
Those two things are the same but said with different words. You are asking for the given parameter (route path) to be relative to CWD (file path); that is what "prefix" means on path resolution.
The disposition of files and directories inside routes is not the only thing that codegen uses to generate the route mapping. Another ingredient to route mapping is the content of .dart files.
Even as it is today, different file paths can generate the same routes, and some file paths (wildcard) will look different when looked at as endpoints. Those are the case of index.dart and wildcards.
These two current DF features create a distance between file paths and route paths. Those create edge cases.
Any new features that create more of that distance will also create more edge cases to be addressed by the new command.
The argument of CLI UX is at least a valid concern but debatable. See the last paragraph of this comment. Also, see items 2 and 3 and say if it is worth it.
Conclusion of 1: It is clear that the extension motivation doesn't hold any water. We are considering this because we are working with prompt URIs, which are not the only way to implement the "new route" feature on extensions. It is clear that the concept of a route is relative to a server router, not file paths, and this addition (with stdin prompt or not) will create a compromise with that association that will be hard to get free from when we need it.
routes/travels/routes/airplane/[id]The closest "routes" dir is not the DF root project. The new command would have to use more heuristics to define where exactly that is.
routes and new route parameter starts with ..As mentioned, we can say "no" to such inputs. The problem is that even if we ignore the incoherence of accepting only some "relative" paths, we limit adding a "dart_frog move route" (and any similar) command without creating even more incoherence.
routes/hello/ and the command is new route with parameter there/../is/../../troubleThe parameter resolves to outside "routes". If we disconsider any relative segment (.., .) from the get-go, we will lose support valid cases (in this cwd, there/../we/go is a valid input, for example). Since this affects the new commands even if running from the root of the repo, we have a ⚠️ breaking change.
We can decide to make a path resolution, but then we move away from the "prefix" argument, and there are new checks.
routes/hello, there is already a file on path routes/hello/index.dart, and the command is new middleware with parameter .Are we accepting . as a valid route path? In this case, we will not use cwd from routes as a prefix for the route as you describe, but actually, we will use path resolution (path.join). We must check any relative segments(.., .) in the provided input and correctly resolve to inside "routes".
routes/hello, there is no routes/hello/index.dart, and the command is new route with parameter .Ditto in case 4.
Conclusion of 2: These are just cases I can think of from the top of my mind. There can be more. I want to let you know that the intention here is to show that this will need some thinking before proceeding, and even after that, be prepared for bug reports.
I am so tired of writing articles on this issue that I will be quick here.
Any project-management feature we add to the CLI (or any related tooling) must consider the added complexity. A dart_frog move concept, for example, would get much more complex if we consider route paths relative to cwd.
Examples of cases that we don't need to cover on DF as is vs. after this mistake feature is added:
⚠️ This is to illustrate the increased complexity of new features that deal with route paths as parameters, don't be too hung up on the "move" merit to not deviate from the issue subject. ⚠️
dart_frog move route "../somewhereelse" "."dart_frog move "." "/rootthingy"That doesn't even consider the added complexity of the daemon, which has a route monitor instance always associated with a dart frog project root path.
As someone who implemented the new command and is currently dealing with the daemon, the juice is not worth the squeeze. being "squeeze," the work of creating a design doc with edge cases, changing the brick and CLI, and after this is added, maintaining dart frog with this.
PS: @erickzanardo, there is no "dart_frog new route or a middleware. Those two commands behave very differently regarding what files are created and, most importantly, moved.
PS2: this is my last comment on this thread, I exceeded my capabilities of being clear on this subject.
renancaraujo
commented
1 year ago Okay, I lied about that one being my last comment.
I may have a solution to address the extension/plugin concern on the daemon front: we can add a method to discover the "dart frog project root path" given a provided directory. Then the IDE can call that, store the result, and pass it to all commands (start, reload, stop, monitor, diagnostic, fix, new route, move route, whatever) that could receive that value.
But this deviates from this issue so I will add it to the daemon design spec.
wolfenrain
commented
1 year ago After reading through this whole thread (boy did it get more comments since I last looked at it) and evaluating both sides I have to agree with @renancaraujo here.
Defining that there is a difference between a Route and a Directory from the CLI point-of-view, as both developers and users of said CLI will save us, as maintainers, a lot of headaches and will keep our code clean/manageable. Yes there would be a loss in user-experience and a bit more logic in other parts like the extensions but in my opinion those are negligible.
If we can save ourselves a lot of headaches by just having a simple resolving logic in the places where it is needed instead of having a N amount of ways to "define" a route in the CLI and potentially making it worse for us and the user then I will stand behind that approach.
alestiago
commented
1 year ago Thank you all for participating in this discussion and getting involved!
Currently, I'm confused with some items of the discussion. Either I'm missing something trivial, I'm miscomunicating or misinterpreting some of the comments. For example, none of the too many edge cases to address are related to the scope of this issue (i.e. not edge cases of this issue).
I'm up for an off-GitHub chat to try to understand the concerns about this and clear my confusion 🙌🏻.
Description
The
dart_frog newcommand should consider the working directory for constructing the route.Currently the
dart_frog newcommand only works from the route of the project. I would like to rundart_frog newfrom inside any directory insideroutes/.Example
Given working directory
routes/items/tea/:Expected behaviour: Creates routes/items/tea/herbal.dart
Current behaviour: