privatenumber
commented
2 months ago Will pick up the work in this PR
Open roryabraham opened 4 months ago
privatenumber
commented
2 months ago Will pick up the work in this PR
roryabraham
commented
2 months ago Awesome! Hopefully what I had here helps 🙂
privatenumber
commented
2 months ago @roryabraham Would you mind reviewing this PR whenever? I plan to release this soon but no rush—feel free to review even afterwards.
You can test it with
npx 'privatenumber/link#npm/publish-watch'(FYI GitHub Actions is stuck right now)
roryabraham
commented
2 months ago So I've tested this with my full e2e testing flow and it seems closer than what I had before. If I run:
npx 'privatenumber/link#npm/publish-watch' publish --watch ~/react-native-live-markdownthen the files in ~/react-native-live-markdown are correctly linked to node_modules/@expensify/react-native-live-markdown.
When I change files in ~/react-native-live-markdown, I've got a nodemon process that will re-run the build of that dependency. npx link will correctly observe that the file changed, but it seems like there's some race condition, and I end up with an error:
Error: ENOENT: no such file or directory, lstat '/Users/roryabraham/react-native-live-markdown/lib/module/MarkdownTextInput.js'manually inspecting my filesystem a moment later, I see that file does exist.
I'm thinking what's happening is something like:
npx link notices the change and begins re-linking the file~/react-native-live-markdown/lib in my case) and starts writing new compiled files into itnpx link tries to hardlink to a file in ~/react-native-live-markdown/lib that isn't present yet. Then it crashes.roryabraham
commented
2 months ago Maybe a solution would be to wrap the call to throttledHardlinkPackage on line 121 in link-publish-mode.ts in a try/catch that will essentially poll for the expected source file and wait for it to show up. And if it doesn't, maybe just log a warning but don't crash the watch process (that way, if someone intentionally deletes a source file in a linked dependency, it won't crash npx link)
privatenumber
commented
2 months ago Thanks @roryabraham
Can you try it again?
I pushed the following changes:
npx 'privatenumber/link#npm/publish-watch' publish --watch <pkg>Having a similar issue as before:
12:46:54 PM Detected rename in ../../react-native-live-markdown/lib/commonjs/MarkdownTextInput.web.js.map
Error: ENOENT: no such file or directory, lstat '/Users/roryabraham/react-native-live-markdown/lib/typescript/src/MarkdownTextInput.web.d.ts'To make sure I got the up-to-date code, I'll try checking out this branch and running link locally rather than over npx
roryabraham
commented
2 months ago Running it locally (so I for sure was using the up-to-date code from this branch). This time I got a different set of errors:
privatenumber
commented
2 months ago Yeah looks like your first request is cached.
Are all those watches being triggered from 1 build? If so, maybe debounce is preferable than throttle.
Since you're running the code directly, are you able to trace back which lines those errors are being thrown from? Feel free to commit a try-catch if you're able to find it.
BTW you should use package.json#files or .npmignore to filter out the non JS files in @expensify/react-native-live-markdown. This will minimize the published package size and reduce the logs on your screen.
roryabraham
commented
2 months ago If so, maybe debounce is preferable than throttle
This makes sense to me - wait for the all the changes to watched files to "settle" before re-linking 👍🏼
Still, relying on that sounds like a flaky solution so I think it would be good to try to get this working with throttle first, then switch to debounce
privatenumber
commented
2 months ago You'll have to do it since I can't reproduce the errors you're getting. Feel free to push to this PR.
roryabraham
commented
2 months ago working on it
roryabraham
commented
2 months ago So while the debounce alone was enough to fix the issues in my case, I managed to fix the error without debounce by adding a simple polling mechanism to wait for source files to show up where they're expected. I set the max timeout to 15s. I also kept the debounce in place because it's more efficient and less noisy.
Maybe it would be beneficial to offer some configurations and documentation for these values. Something like this...
--watch - create a watched hard-link for a package. This can be useful for certain build systems that may delete build files before overwriting them. This may break hard-links, so this flag will create a file watcher to re-establish broken hard links.maxBuildTime. If using the --watch flag to link a package in publish mode, what's the maximum time you expect to wait between files being deleted by your build system and having them replaced?pollingInterval. If you are using the --watch flag and this package detects a broken hard link, how frequently should it poll to see if the source file for the hard link is re-established?what do you think?
roryabraham
commented
2 months ago At this point this PR is working for my needs. thanks for the help pushing it forward @privatenumber. let me know if you have any feedback or if you think it's worth adding those additional configs I mentioned in the last comment
roryabraham
commented
2 months ago update: actually, there are more race conditions at play here. Namely, we're executing getNpmPacklist right after the debounced watcher is triggered. This will take an inventory of files by traversing the source package. But if some of those files have been deleted and not re-written yet, then we get an incomplete list. this is further challenged by the fact that we can't simply wait for the files to appear, because there may be some cases where a file in a linked package is intentionally removed by a developer.
roryabraham
commented
2 months ago ok, I've found a more robust solution. But it requires some work on the side of the linked package. The idea is that in the linked package, you can update your build process to:
.build_complete file at the start of each build.build_complete file at the end of each buildlink can reliably wait for the .build_complete file to appear before running getNpmPacklist and re-establishing hard linksroryabraham
commented
2 months ago Updated this PR with an implementation that is hopefully both flexible and approachable. Again, I'm open to feedback. Thanks
privatenumber
commented
2 months ago Really appreciate your work on this @roryabraham, but I'm not sure if I want to add integrations to that extent. Specifically, I'd like to only support the --watch flag, and would prefer not to add things like a custom file for the build to signal to link with.
I'm curious what kind of build you're running that causes so many race conditions? How long does 1 build iteration take that the debounce fires before the build is done?
In any case, I think it's fine to just do debounce and check if the file exists right before hardlinking and ignore if it no longer exists. If the build is still running, a second debounce pass should run to fix it. Does that sound right?
To make it a little easier for the user to manually intervene on missed changes, we can also listen to the Enter key on stdin to retrigger the link.
roryabraham
commented
2 months ago Well, really there's only one race condition, it just took a while for me to understand the root cause. It exists between getNpmPacklist and the completion of the build.
I'm curious what kind of build you're running that causes so many race conditions? How long does 1 build iteration take that the debounce fires before the build is done?
I think my build typically takes 2-5 seconds, so a 500ms delay doesn't serve our use case very well. My experience with JS packages (using babel and such for minified builds) is that they rarely compile in <500ms. YMMV, as it may depend on hardware too. I'm working on an M2 MacBook Pro, so it feels unlikely that my hardware is a limiting factor.
People will have lots of different experiences with build times though, so I'm not sure debating what an "average build time" is will be productive.
If the build is still running, a second debounce pass should run to fix it. Does that sound right?
Unfortunately, I don't think this is correct. The reason is that if getNpmPacklist runs when the build is incomplete, then our watcher will stop watching files that are missing from the package manifest. So once the missing files appear, the watcher will ignore them.
This kind of cuts to a core challenge - without a specific file to use as a litmus test for a complete build, we have no way to tell the difference between files being missing and legitimately deleted.
If you're looking for a way to simplify the implementation, here's an alternate idea... we could just ignore that distinction and say that we don't support deleting files in the dependency package (i.e: delete this block). In practice, if we're linking a file that you've deleted, it won't be a problem because in order to legitimately delete a file you also need to delete code references to it.
What that means is that npx link publish --watch would support new files being added and returned by getNpmPacklist, but not deleted. That would improve things in many cases, because we could wait for all expected files to appear before re-linking. However, that still would not be immune to race conditions. For example:
A, in dependency packagegetNpmPacklist doesn't return the compiled version of the new file A' in the manifest, because it hasn't been written to the output folder yetA' is missing from the manifest we don't wait for it to appear or link it 💥All this is to say, I really do think that relying on something like a .build_complete file to appear in the dependency package would be the most robust and reliable solution. I also tried to set up some sensible defaults such that it should work for most people without having to do that, but also included documentation for my recommended approach.
Ultimately, it's your call what you want to do with the package. Happy to discuss further, or if you're confident in what you want to do, I can update the PR accordingly.
privatenumber
commented
2 months ago People will have lots of different experiences with build times though, so I'm not sure debating what an "average build time" is will be productive.
I'm not sure where this came from but I was just curious to learn. Keep in mind that I don't experience the issue you're encountering and I haven't been able to reproduce it. That said, it's difficult for me to validate or ideate solutions without fully understanding the problem.
Would it be hard to create a test case to reproduce this race condition?
roryabraham
commented
2 months ago Yeah, absolutely I can try to work on a minimal reproduction to more clearly illustrate the problem(s) I'm describing. 🙂
My sincere apologies if I sounded confrontational with that comment - that wasn't my intention at all. I merely meant to say that choosing the "appropriate debounce" will likely vary based on the situation.
privatenumber
commented
2 months ago Any progress?
No rush btw. If you're unable to find time for this, LMK and I can release a basic version of this and you can open an improvement PR at your leisure.
roryabraham
commented
2 months ago Hey, sorry I haven't come back to this. Very busy with other work right now, so it's unlikely I'll be able to get back to this in the next few weeks
privatenumber
commented
2 months ago No worries at all, I've been busy too. Thanks for letting me know!
Fixes https://github.com/privatenumber/link/issues/22
This PR adds a
--watchflag to the publish command. If the source file is deleted and re-created (as is the case for some build systems, such as https://github.com/callstack/react-native-builder-bob), then the--watchflag will re-establish the hard link with the new file in the source file path.