-
Notifications
You must be signed in to change notification settings - Fork 139
Move postInstall commands to their own script; add command to kill metro #247
Conversation
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.
brilliant
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.
Really awesome! Also, moving postInstall to a custom bin
command was brilliant. Much easier to read than all the inline things we had in there before.
Reporting the following error from Windows 10 x64 install attempt:
PS: Came here from the Edited to add: the fix-postinstall-windows branch worked for me. |
Also, fix the Windows platform name.
@COSMOPHILE Thanks for testing this and letting me know about this problem! I've found and fixed that problem (helped by this 2014 (!) blog post from @shapeshed -- https://shapeshed.com/writing-cross-platform-node/#scripts-in-package-json) Does it work for you now? |
Not for me, still getting the same error... |
@COSMOPHILE I just tested again on Windows 10 and it's working for me. I'm surprised that you're seeing the same error as before, since that error ( To be sure, I just tried changing the command to
and also tried changing it to
Neither of these are the same as the previous error, so could you please double-check that you've pulled this branch, through commit 4387965? and see whether some other part of the error message changed (between Again, I really appreciate the help testing this! |
Hey @bryanstearns, really appreciate your investigating this. I double checked my version and it's up to date with your branch, and the error is also only slightly different to reflect that (lastish line):
So I don't know why it's reading the changed string I wish I knew more about the OS to know where to troubleshoot this... Thanks again. |
can you not just merge #245 as a patch and plan this approach for a future release? the current boilerplate does not work on windows, you have a patch that works, you can then surely iron out the kinks for this more sustainable solution for the next minor release? |
I'd like us to merge this: it works for me on Windows, which is better than what's released now, and if it doesn't work for you, maybe you could dig into why it's not working and include that in the issue you open? 🤞 |
Looks like CI is breaking -- looking at it |
It appears related to this issue: storybookjs/storybook#8083 |
## [4.6.3](v4.6.2...v4.6.3) (2019-09-16) ### Bug Fixes * **deps:** Locking storybook version to avoid Storybook issue storybookjs/storybook#8083 ([3aa472a](3aa472a)) * **windows:** Move postInstall commands to their own script; add command to kill metro ([#247](#247) by [@bryanstearns](https://github.com/bryanstearns)) ([d5e878d](d5e878d))
🎉 This PR is included in version 4.6.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This is an alternative to #245, which I think will work better for the long term.
This adds a new
bin/postInstall
script to do all the yarn or npm postInstall steps; it can besmarter about what steps to run on which platforms, more generally than #245 did.
I've also added a new postInstall step: it kills any running instance of the Metro bundler when you install packages. I've had problems when I forget to do this, and it takes virtually no time. Note that it only does this on Mac or Linux because I haven't worked out the equivalent Windows incantation; HELP WANTED if a Windows person wants to try!
I've tested
ignite new MyApp -b ./bowser
on Mac and Windows.Like the other implementation, this should fix #243 and maybe #244.