feat(publish): watch mode #24
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
|
Will pick up the work in this PR |
|
Awesome! Hopefully what I had here helps 🙂 |
privatenumber
changed the title
|
@roryabraham Would you mind reviewing this PR whenever? You can test it with npx 'privatenumber/link#npm/publish-watch'(FYI GitHub Actions is stuck right now) |
roryabraham
commented
Aug 4, 2024
| } | ||
|
|
||
| await hardlinkPackage( | ||
| const throttledHardlinkPackage = throttle(hardlinkPackage, 500); |
There was a problem hiding this comment.
It's not immediately clear to me why you chose to throttle this function. Maybe a code comment would be a good idea here.
There was a problem hiding this comment.
I also was curious so I investigated a bit the difference between throttleit and p-throttle. I think throttleit is appropriate here 👍🏼, since I assume we want to discard any but the last invocation of hardlinkPackage in an interval
| @@ -38,12 +39,14 @@ | |||
| "execa": "^8.0.1", | |||
| "fs-fixture": "^2.4.0", | |||
| "get-node": "^15.0.1", | |||
| "glob-to-regexp": "^0.4.1", | |||
There was a problem hiding this comment.
Noticed that this dependency is marked as archived/read-only, while a more popular alternative https://github.com/isaacs/minimatch is still actively maintained (and apparently used by npm internals).
Was there some reason you chose glob-to-regex instead?
| continue; | ||
| } | ||
|
|
||
| const shouldIgnore = ignoreFiles.some(ignoreFile => ignoreFile.test(filename)); |
There was a problem hiding this comment.
Probably doesn't matter too much, but one thing you could consider if you're sticking with globToRegexp is joining the regexes together then calling test just once rather than in a loop:
diff --git a/src/commands/publish/link-publish-mode.ts b/src/commands/publish/link-publish-mode.ts
index e0b535c..69caad3 100644
--- a/src/commands/publish/link-publish-mode.ts
+++ b/src/commands/publish/link-publish-mode.ts
@@ -79,7 +79,7 @@ export const linkPublishMode = async (
* npm-packlist ignore list:
* https://github.com/npm/npm-packlist/blob/v8.0.2/lib/index.js#L15-L38
*/
- const ignoreFiles = [
+ const ignoreFiles = new RegExp([
// Files
'**/{npm-debug.log,*.orig,package-lock.json,yarn.lock,pnpm-lock.yaml}',
@@ -91,7 +91,8 @@ export const linkPublishMode = async (
// Hidden folders
'**/.{_*,git,svn,hg,CVS}/**',
- ].map(glob => globToRegexp(glob, globOptions));
+ ].map(glob => globToRegexp(glob, globOptions).source)
+ .join('|'));
const watcher = fs.watch(
absoluteLinkPackagePath,
@@ -103,7 +104,7 @@ export const linkPublishMode = async (
continue;
}
- const shouldIgnore = ignoreFiles.some(ignoreFile => ignoreFile.test(filename));
+ const shouldIgnore = ignoreFiles.test(filename);
if (shouldIgnore) {
continue;
}
|
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 When I change files in manually inspecting my filesystem a moment later, I see that file does exist. More logsI'm thinking what's happening is something like:
|
|
Maybe a solution would be to wrap the call to |
|
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: 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 |
|
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: logs |
|
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 |
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 |
|
You'll have to do it since I can't reproduce the errors you're getting. Feel free to push to this PR. |
|
working on it |
|
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...
what do you think? |
|
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 |
|
update: actually, there are more race conditions at play here. Namely, we're executing |
|
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:
|
|
Updated this PR with an implementation that is hopefully both flexible and approachable. Again, I'm open to feedback. Thanks |
|
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 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. |
|
Well, really there's only one race condition, it just took a while for me to understand the root cause. It exists between
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.
Unfortunately, I don't think this is correct. The reason is that if 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
All this is to say, I really do think that relying on something like a 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. |
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? |
|
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. |
|
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. |
|
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 |
|
No worries at all, I've been busy too. Thanks for letting me know! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #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.