search

broccolijs / broccoli-persistent-filter

MIT License
12 stars 33 forks source link

Updates to dependency tracking to work with the fs-merger. #191

Closed chriseppstein closed 4 years ago

chriseppstein commented 4 years ago

This PR builds on top of the typescript port in PR #190. You should only review the commits starting with Use the fs-merger facade in dependency tracking. in this PR.

This PR also requires that we land https://github.com/SparshithNR/fs-merger/pull/35 and require that new version to be used. This project doesn't have a production dependency on the fs-merger package so I'm not sure what the best way to accomplish that is (maybe a runtime check? maybe a peerDependency? Maybe release a new version of broccoli-plugin that bumps the dependency spec for fs-merger).

Overview of Changes

The dependency tracker was conceptually well aligned to use the FSMerger because it tracked files outside the tree separately from files inside the tree. The dependency tracker allows relative paths which are assumed to be relative to the file the dependencies are being registered against. It also allows absolute paths to be provided as dependencies. The public API for setting dependencies made no distinction for the caller about "paths in the tree" vs "paths outside the tree", instead it sorted that out for them. Given that broccoli-plugin still exposes inputPaths, I think we have to be able to make sense of absolute paths in general without putting that onus on the API user.

As noted above, I've submitted a PR to the fs-merger to allow me to take an absolute path and check if it's part of any input tree. I think this is a good compromise since it doesn't expose the root paths of the fs-merger, rather it will only confirm them if the caller happens to have a reference to one of the root paths.

You'll note that the new utility function resolveRelative raises an error if you try to create a relative path that escapes the local filesystem root. I feel like this is an important abstraction boundary and I worry about trees generally trying to read from other trees that aren't in the input trees if we don't create a firm boundary around reading relative paths through the fs-merger (fwiw, the fs-merger doesn't currently enforce that boundary -- I think that's a bug, the semantics of reading ../foo.txt from all of the input paths to the merge seems ill-defined to me).

The result of not allowing relative paths to escape the input tree technically constitutes a breaking change. But I don't think that in practice it will be one, since the dependency API is not widely used (I might be the only user and I never do this in any of my code). I leave it to your judgement about whether you should bump the major version because of this.

This PR also changes the serialized form of dependencies . I handle this in the code by refusing to deserialize any cached dependency tracking from older versions of the code. This will cause the first build after upgrading to be slow (it was probably already invalidated through other means but I wanted to be sure I wouldn't cause an error).

Let me know if you have any questions or concerns.

chriseppstein commented 4 years ago

Note: The tests are failing because the dependency on https://github.com/SparshithNR/fs-merger/pull/35 hasn't landed. It will pass locally if you yarn link my branch to this code.