olmps / memo

Memo is an open-source, programming-oriented spaced repetition software (SRS) written in Flutter.
BSD 3-Clause "New" or "Revised" License
1.82k stars 158 forks source link

Add first `FileSystemGateway` proposal #200

Closed matuella closed 2 years ago

matuella commented 2 years ago

What are the plans of usage for this gateway? I had a lot of problems using process.cwd and other relative paths builder like __dirname and stuff like that and ended removing all of them due to several different reasons. I don't see the usage use-cases in Memo today for such gateway (if there is, please let me know).

Which problems did you had? But then, how did you accessed the filesystem?

Thisz will be used to read all local collections to make diffs, just like we are doing with flutter today.

This is problematic, since process.cwd is relative to the path that the process originates. If this gateways is invoked from scripts/, it will have a different path than if invoked through functions/ as an example. See my general comment below

Isn't it in relation to the file itself? Meaning, this filesystem-gateway output? I think this should be a clear decision on the strategy itself, but I don't see how we run away from relative paths other than maybe changing the requirements to be relative to the node's process root folder, meaning package.json. But I'm not sure if expecting an absolute or relative is any different here other than it should be more explicit.

ggirotto commented 2 years ago

Which problems did you had?

I had problems using process.cwd when invoking from different project folders, like from functions/ and from /scripts, and the output was different.

Thisz will be used to read all local collections to make diffs, just like we are doing with flutter today.

Hmm, that's right. We need to access filesystem somehow.

Isn't it in relation to the file itself? Meaning, this filesystem-gateway output? I think this should be a clear decision on the strategy itself, but I don't see how we run away from relative paths other than maybe changing the requirements to be relative to the node's process root folder, meaning package.json. But I'm not sure if expecting an absolute or relative is any different here other than it should be more explicit.

I would ask for you to test if it works fine when invoked through other folder than src/core. The problems I faced may not happen here due to the project folders structure, what would be awesome. But if it faces the problem I mentioned above, I think we would need to think in something else, not sure what.

matuella commented 2 years ago

I would ask for you to test if it works fine when invoked through other folder than src/core. The problems I faced may not happen here due to the project folders structure, what would be awesome. But if it faces the problem I mentioned above, I think we would need to think in something else, not sure what.

What exactly do you want me to test? I do get your point, but for what you're asking, I think it's better to simply create integration tests and make sure that it's working as expected (indirectly testing fs) - instead of wondering about possible issues that we are not sure of.

matuella commented 2 years ago

@ggirotto just making sure that you're being notified, because you haven't added a review.

ggirotto commented 2 years ago

What exactly do you want me to test? I do get your point, but for what you're asking, I think it's better to simply create integration tests and make sure that it's working as expected (indirectly testing fs) - instead of wondering about possible issues that we are not sure of.

But your integration tests will not catch this specific issue, since the filesystem will be mocked as well. What I'm saying is that I faced problems when using the same gateway from different source folders, because process.cwd had a different output. As an example, if we want to write a script that load all collections from the fs and do some stuff with it, would process.cwd emit a different output - since we're running it from scripts/ folder - and make the use-case fail because of that?

Sorry about not being concrete enough, it's just that I had this exactly problem when using process.cwd, and I think it may happen in this project as well.

matuella commented 2 years ago

But your integration tests will not catch this specific issue, since the filesystem will be mocked as well

I meant testing the integration between this gateway and the fs, or maybe wait for everything to be setup to actually create a wider e2e test.

if we want to write a script that load all collections from the fs and do some stuff with it, would process.cwd emit a different output - since we're running it from scripts/ folder - and make the use-case fail because of that

I may be missing something, but then whoever calls it, should take into consideration where it's being executed. Relative or absolute paths have their problems, but using one or another will not change the fact that the node process could start running from different root folders.

See, these files can be literally anywhere within the node's process working directory, it's up to the FileSystemGateway caller to know where they are located, but this process current working directory will be always pointing to root, different from __dirname, which uses the path from the script that has called it. Alternatives to this assume hardcoded paths and I don't think it is the way to go.

matuella commented 2 years ago

All updates made here will be merged in the last PR (#211), in this nested chain of collection execution PRs.