-
Notifications
You must be signed in to change notification settings - Fork 221
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
fix(cli): Make Shortest compatible with Windows #256
Conversation
@PedroAVJ is attempting to deploy a commit to the Antiwork Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PedroAVJ are the changes from this file needed for Windows compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also had to fix some issues with page.evaluate. Apparently __name is not defined due to how esbuild works, and was causing the tests to crash.
I could remove the extra changes I made but, at least at the time, I couldn't then run the tests to ensure everything worked fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm noticing the same error when testing the package using the source code, e.g. via :
pnpm pkg:test:src
Have you tried building the package first?
pnpm pkg:build
pnpm shortest
I'd rather address this issue in a separate PR, and keep this one only for fixing the Windows compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, I don't think I did, but can't fully recall either. Either way you are right. No need to block this issue any further
…code readability" This reverts commit 29cfe0b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @PedroAVJ.
I've reverted the changes for browser-tool.js
so we can address the issue separately.
@@ -21,7 +21,7 @@ | |||
"dist/cli" | |||
], | |||
"scripts": { | |||
"build": "rm -rf dist && pnpm build:types && pnpm build:js && pnpm build:cli", | |||
"build": "rimraf dist && pnpm build:types && pnpm build:js && pnpm build:cli", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm -rf
for Node in a cross-platform implementation
Fixes #251
I also had to fix some issues with page.evaluate. Apparently __name is not defined due to how esbuild works, and was causing the tests to crash.
I have tested that the cli works when running pnpm build:pkg and than pnpm shortest --headless, on both windows and linux