Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[yarn rwfw]: clean before building, don't exit on errors #3074

Merged
merged 6 commits into from
Jul 21, 2021

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Jul 19, 2021

All this PR does is add a cleaning step before building.

Currently, if you edit a typescript file while yarn rwfw project:sync is running, you'll most likely run into issue #3053. The reason for this is that there's a bug in ttsc: it's incompatible with the version of typescript we're using (see this comment here: #3053 (comment)).

ttsc hasn't been updated in a while so it seems unlikely that this issue will be fixed. As @Tobbe said in the comment linked above, migrating to ts-patch v2 seems like the long-term solution. The issue there though is ts-patch v2 is in beta. But we could certainly help out the project by becoming an early tester.

Since there's some uncertainty about ts-patch / work would have to be done to see if it's viable, this PR provides a temporary fix to make contributing easier in the meantime. All this PR does is add a cleaning step before building.

This PR also ignores a new filepath, dist (without the trailing slash). Since we're listening to all chokidar events, one of which is addDir, now that we run build:clean, the addDir event fires on build (a new directory dist was added), causing it clean and build, over and over again. Ignoring dist fixes that.

Another option that was discussed was catching the error and consoling a helpful message. I'm less inclined to go that route since the goal of a contributing tool is to ease friction and let the contributor just contribute. Having to run build:clean yourself after every change sounds a little too tedious.

@jtoar jtoar requested review from peterp and dac09 July 19, 2021 20:55
@jtoar jtoar self-assigned this Jul 19, 2021
@dac09
Copy link
Contributor

dac09 commented Jul 20, 2021

@jtoar - what I was suggesting re: capturing the errors is the following:

On error, capture the error, and display an error message to the user (could not build & sync ${package}) . This means that if you have some sort of error in the code you're modifying (which is possible, and common when you're devving, especially with types) - the watcher continues to run, and I don't have to rerun rwfw to go through the lengthy process of rebuild everything, link everything, etc.

This would be in addition to clean before build change you've just done.

@dac09
Copy link
Contributor

dac09 commented Jul 20, 2021

  • Change script to clean at a per-package level

@thedavidprice
Copy link
Contributor

Per conversation, #3082 will make "clean before building" unnecessary (since the original need was addressing a bug with ttsc package). Regardless, we would still use this PR for improved error catch/output.

If we do need to proceed with the "clean before building" approach, then it will be important for performance to only re-build changed packages.

@thedavidprice thedavidprice added this to the next-release-priority milestone Jul 20, 2021
@jtoar jtoar changed the title [yarn rwfw]: clean before building [yarn rwfw]: clean before building, don't exit on errors Jul 21, 2021
Comment on lines +95 to +100
try {
buildPackages([packageJsonPath], { clean: true })

console.log()
logStatus(`Copying ${packageName}...`)
copyFrameworkFilesToProject(projectPath, [packageJsonPath])
console.log()
logStatus(`Copying ${packageName}...`)
copyFrameworkFilesToProject(projectPath, [packageJsonPath])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's an error building a package, it doesn't make sense to proceed in copying it over so I included it in the try-catch block

Comment on lines +150 to +154
if (clean) {
for (const packageJsonPath of packages) {
rimraf.sync(path.join(path.dirname(packageJsonPath), 'dist'))
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rimraf.sync doesn't take arrays of filepaths until v4 so we'll have to loop it for now

@thedavidprice
Copy link
Contributor

Discussion reminder: add yarn rwfw --help|help, which would output the same as yarn rwfw.

@jtoar
Copy link
Contributor Author

jtoar commented Jul 21, 2021

@thedavidprice done thanks to @alicelovescake! 🚀

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍰

@dac09 dac09 merged commit 92f0956 into main Jul 21, 2021
@dac09 dac09 deleted the ds-handle-ttsc-build-error branch July 21, 2021 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants