sysgears / spinjs

SpinJS is now Zen! The project has been renamed and moved to Larix Framework.
https://github.com/sysgears/larix/tree/master/packages/zen
MIT License
43 stars 8 forks source link

Adding support for expo-cli #12

Closed haysclark closed 9 months ago

haysclark commented 6 years ago
larixer commented 6 years ago

@haysclark Thank you Hays. How about making this a non-breaking change and supporting exp and expo at the same time? Btw, does this implementation really work?

haysclark commented 6 years ago

@vlasenko Yes, I am happy to change the logic to support both. It's would be pretty trivial. I have not been able to validate that the 'expo' version works.

In my apollo-universal-starter-kit project I have used:

 "dependencies": {
    "spinjs": "git+ssh://git@github.com:infinitedescent/spinjs.git#feature/expo-cli"
  },

Unfortunately, I am getting errors:

/Users/hays/.nvm/versions/node/v10.11.0/bin/node /Users/hays/.nvm/versions/node/v10.11.0/lib/node_modules/yarn/bin/yarn.js run expo start
yarn run v1.10.0
$ yarn --cwd packages/mobile expo start
$ cross-env NODE_ENV=production spin expo start
[22:13:33] Starting project at /Users/hays/infinitedescent/ocean-hub-service/packages/mobile/.expo/all
[22:13:35] Metro Bundler process exited with code 0
[22:13:35] Set EXPO_DEBUG=true in your env to view the stack trace.
infoerror Visit https://yarnpkg.com/en/docs/cli/run Command failed with exit code 1.
 for documentation about this command.
infoerror Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
 Command failed with exit code 1.

Is there a good way to make a simpler test case?

larixer commented 6 years ago

@haysclark Yes, it means this implementation is not working. The implementation should be much more sophisticated to support expo command.

haysclark commented 6 years ago

@vlasenko I updated the spinjs to support both the original exp command, and the newer expo command. As you can see I simply forked startExp() and added startExpo(), which is pretty much the same code as startExp(). I figured it would be better to have seperate code flow over a more DRY solution.

I tested the code today and everything seems to be working, or at least it works as well as apollo-universal-starter-kit: stable works.

Test cases and results:

For all three repo's yarn watch runs perfectly. However, in all of the repo's yarn exp start or yarn expo start does not startup the Metro Bundler like calling exp start or expo start would. Additionally, yarn exp ios and yarn expo ios will start up the iOS simulator; however, the Metro Bundler is not running in the background.

So, thus far I have not seen any regression of behaviour. Please let me know what you think or if you have any question or comments.

larixer commented 6 years ago

@haysclark Of course this approach won't work. You have verified scenarios of using exp command that were not used in apollo-universal-starter-kit and that were not supported. Of course, after your changes they are still not supported.

How about verifying scenarios that were supported and were used? You can easily see them in startExp implementation that you have copied: https://github.com/sysgears/spinjs/blob/1fcf43503539390199c5b6f26ad9d08aa94524ed/src/executor.ts#L1000 This line contains the list of commands that have special support from spinjs, other commands are just executed as is and might not be supported yet by spinjs.

You should verify instead support of following exp commands by spinjs: 'build:android', 'build:ios', 'publish'

haysclark commented 6 years ago

You should verify instead support of following exp commands by spinjs: 'build:android', 'build:ios', 'publish'

@vlasenko Thanks for the advice and for your patience! I am still ramping my brain around the entire apollo-universal-starter-kit and the role that spinjs plays in it. I was just trying to get into the project and start contributing instead of just posting an issue and expecting someone else to implement it. I really appreciate you taking the time to point me in the right direction and I apologize if my confusion has been frustrating.

I created an issue in the [apollo-universal-starter-kit(https://github.com/sysgears/apollo-universal-starter-kit/issues/878)) to help capture how I got confused as to how Expo is intended to be used with the project.

If possible, could you make a branch for me to look at in regards to #14? I am just hoping to implement some pinning tests, and want that effort to match how you would want tests set up in your project. Either that has tests set up the way you prefer, and I can integrate them myself.

larixer commented 6 years ago

@haysclark I have added a couple of unit tests via Jest as an example via commit: https://github.com/sysgears/spinjs/commit/ba8a348d7967c7c40c58893de9fb8a8ddbad7194

I hope this helps