| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
Thanks for the PR! We're using the hello name without the extension for the Windows commands in a couple more places in the documentation. Would be nice if we could update those too -
Also, do you mind using doc instead of docs for the first commit? The commit validator in CI would fail otherwise and this would require a manual landing then. It's fine if you don't wanna do it, I can handle it while landing. :) |
Sorry, something went wrong.
|
The step reference of copying node executable may be wrong, and I also fix it in 00454f5 |
Sorry, something went wrong.
|
Another question is in some windows with powershell, the default encoding is UTF-16. That is to say in the step 1 and 2, the encoding of echoed hello.js and sea-config.json is UTF-16. I am not sure whether it will cause some unintended errors. (I have no environment to test it now) |
Sorry, something went wrong.
|
I think UTF-16 too should be okay because we have a test that uses a UTF-16 character in the SEA - Line 35 in 595b2b3 |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
I reckon where.exe node also gets the job done |
Sorry, something went wrong.
It works in cmd. I also searched equivalent of Bash's $(), and write the following command to copy node, but I am not sure whether there is a better or eleganter command in windows cmd. for /F "tokens=*" %n IN ('where.exe node') DO @(copy "%n" hello.exe) |
Sorry, something went wrong.
|
@nodejs/platform-windows any thoughts on ^? |
Sorry, something went wrong.
|
@yjl9903 FWIW, if this works on cmd, I'm in favor of using this command for the Windows docs because then we'll have a command that would work both on cmd as well as powershell on Windows. If someone else knows of a more elegant way of doing this, maybe that could be done in a separate PR later? |
Sorry, something went wrong.
Done. |
Sorry, something went wrong.
There was a problem hiding this comment.
Nice changes! Thank you for the first contribution :)
Sorry, something went wrong.
|
@yjl9903 unfortunately, our tooling doesn't support merge commits yet, so you might need to go for a rebase instead |
Sorry, something went wrong.
|
Oh, I think I am not very familiar with how to do rebasing... How to reopen this PR @RaisinTen ? |
Sorry, something went wrong.
|
@yjl9903 could you try checking if https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c helps? So IIUC, you would need to run: git push -f origin b31d587dc8c1b0f459243d2b96a17a7175c996cf:patch-1
# Reopen the PR.
git push -f origin 45c4698fa56d28579009de9d78a1f41b372689ec:patch-1Alternatively, you could also try to click on "Create pull request" in main...yjl9903:node:patch-1, which would help you create a new pull request with the current list of commits that are present on your branch. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Hello.
In single-executable, copyied node executable on windows should be named with extension .exe, this PR added some guides about it.
Related discussion nodejs/single-executable#65