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

tech: upgrade to 6.0 (part 1) #135

Merged
merged 26 commits into from May 11, 2021
Merged

tech: upgrade to 6.0 (part 1) #135

merged 26 commits into from May 11, 2021

Conversation

dannyhw
Copy link
Member

@dannyhw dannyhw commented Nov 16, 2020

Issue: #131

What I did

Work underway for 6.0!

More details to come.

@dannyhw
Copy link
Member Author

dannyhw commented Dec 10, 2020

So heres the current progress on this:

Done:

  1. Basic project working with 6.0 dependencies
  2. We currently removed the server code from @storybook/react-native package for now to simplify the upgrade. This is mainly because the server code would make it much harder to upgrade. We may include it again later however I'm thinking that we should focus on improving on-device UI (open to feedback on that).
  3. Added an example project for expo web (v39) to use for quick iteration and for making sure web is working.

In progress:

  1. auto import stories based on glob patterns. This has been a difficult one to solve because of metro's issues with symlinks and weird quirks like not importing files that are hidden (start with '.') also another issue we found when trying to use babel pre-val for this was that the cache would not update. However we were able to show that it could work with a lightweight watcher that creates a "requires" file. This would be kind of similar to how the loader project works (search react native storybook loader) however it would watch for changes and should work with hot reloading.
  2. refactoring things
  3. experimenting with a new storylist component that allows for categories (the story list in the sidebar)

To Do:

  1. 6.0 addon support
  2. everything else

Blockers:

  1. fast reload support continues to be a bit of an issue, however I have some theories about how we can resolve this.
  2. react hooks don't work in the mono repo for some really frustrating reason and I've already tried multiple times to fix this, continuing to attack this issue.

Also you may have noticed that expo 40 came out so I will soon update the expo examples to use that and test compatibility.

@dannyhw
Copy link
Member Author

dannyhw commented Dec 19, 2020

TL;DR progress made using chokidar and glob to add automated story imports

Made progress today with auto importing stories, this had to be a little more complex than I was hoping due to some of the stuff I mentioned in the previous update.

Basically it works similar to the react native storybook loader except we use a watcher so that it will automatically re-write a file that contains all the requires. With the loader when you add a new file it doesn't re-write the requires unless you re-run the script manually. With the watcher I've added you can run another process along side the metro bundler and new files should automatically get picked up. You can run the pre-script instead if you aren't adding new stories very often.

The work remaining for this watcher is

  1. refactor the code
  2. make it more automatic and easier to setup

This work is related to CSF support by adding the main.js config file, basically we should support the 3 config files that web storybook uses so that we can re-use the csf preprocessing code. Possibly the watcher code could be extended to run the csf stuff.

Next I'm thinking to spend some time (again) to try and resolve the issue with hooks in the mono repo so I can refactor the project to be more modern.

@MasterAM
Copy link

Hi @dannyhw and thanks for your work on this update.

We've been using Storybook with ReactNative for years now, in several projects.

Since I don't enjoy the need to comment and uncomment code to toggle between storybook and the actual app in the entry point, we've always managed to create a separate entry point for the Storybook target and run that one without touching the rest of our code, although we almost always needed to adjust our configuration to make up for metro-related issues.

I don't know how the more automated config method in v6 (I only saw it briefly in the release notes of the web version) would affect the ability to have a separate entry point, but I hope that it would make matters easier rather than making it impossible to override. Do you have any opinion/plan on the matter?

Regarding the server usefulness, I find having the server highly important since we often launch several emulators and physical devices in parallel to test our apps on different screen sizes and configurations, and having the server allows us to switch between the stories on all of the devices at once. I think that this type of use-case makes the tool extremely useful.

@dannyhw
Copy link
Member Author

dannyhw commented Dec 23, 2020

@MasterAM hey thanks for your feedback. It's really good to hear back from people as it's hard to know if I'm taking the right steps without this kind of feedback.

Regarding having multiple endpoints I don't want to make anything worse of course the intention is more of an optional extra. Generally I use storybook as a separate app as part of a component library however I understand that many people want to have it part of their main app. This use case will continue to be possible. I really want to make it easier to swap between the two without increasing app size the major issue is finding a good way of doing it. If you have experience with this suggestions are welcome.

About the server, it good to know of this use case. My concerns with the server is that it's a big maintenance cost and it's something I'm not familiar with. In order to make the server work I'm going to have to invest a lot of time to understand how it works and how that fits in with 6.0 (I'm not the original creator I became maintainer in August). For me it would be really good to know if more people are interested in keeping the server around so that I know it's a priority for the community. That being said if anyone is willing to help work on the server that would make it much easier for me.

The idea initially was to release V6 without the server and then add it back later this because I feel like whilst we are stuck at 5.3 the project gets left behind and a lot of people are waiting for V6. Like I said though this is nothing final and feedback will dictate the direction taken with the project.

Thanks again for your feedback it's very much appreciated.

@MasterAM
Copy link

It's a bit off-topic, but last weekend I had a look at the source of the original storybook repo and this one to try and find out if I could assist with the migration to 6.0.

I found out that the project was recently migrated to TypeScript, which is overall very good, but makes tracing the changes across renames (js -> ts) more difficult.

The main obstacle I came across was creating a proper dev/test environment that would include a properly configured demo app, server and storybook repos (preferably both react-native and core), all linked against each others' local copies. I don't have any experience with Lerna, so I was wondering if there is any set of instructions for building a dev env for those (I only found something for the main storybook repo), or you simply improvise one.

If you could shed some light on this, and if there is a more appropriate comm channel for this, I would be very grateful.

@dannyhw
Copy link
Member Author

dannyhw commented Jan 2, 2021

@MasterAM hey I've been taking a bit of a break for the holidays so didn't see this right away, but I'll respond to you tomorrow. One great way to get in touch is via the storybook discord, there is a link in the readme. I'm active in the react-native channel there.

@dannyhw
Copy link
Member Author

dannyhw commented Jan 2, 2021

@MasterAM please refer to the dev guide this should help clear up some of your questions about how things are setup. The react-native server code is still there I just removed some of the code from app/react-native/src/preview/Preview.tsx (previously index.tsx) that was being used to initiate the websockets and other stuff related to the server.

Here's the discord link

If you want we can organise a call for me to show you the code and explain how some of it works.

@jgornick
Copy link
Contributor

Thank you for working on this! Since we use both web and native versions of storybook for our cross-platform components, I can't wait until we can upgrade 👍

@dannyhw
Copy link
Member Author

dannyhw commented Jan 15, 2021

@jgornick no problem happy to have your support. I was taking a break over the holiday period however I do plan to get back to working on this soon.

And for anyone coming here looking for updates:

To attempt to address the lack of progress more recently I'd like to add a small explanation in case anyone is concerned.
Unfortunately theres a lot of issues with the repository making it difficult to make progress, however I'm thinking about rebuilding the tooling/workspace setup if I can't resolve it soon. I've spent many hours removing an re-installing the npm dependencies trying to solve the hooks issue among other things. Hooks are not necessary at first glance but when you consider the libraries that have adapted hooks it makes it almost impossible to upgrade certain packages like emotion until I can fix this issue. This became very frustrating and I decided to take a break so as to not experience burn out.

I can also say now that I now understand much better how much more work is needed, however I'm not giving up on the project so if you're willing to stick with us then 2021 will be a better year for react native storybook I'm sure.

Also I'd like to mention again that if you want to contribute PR's please do contribute, collaboration is what makes open source worth doing. Also to be clear fixes for the 5.3.23 version are very welcome as long as you can avoid breaking changes (those would need to go in 6.0). And on that note there should be a minor patch with some small fixes soon (5.3.24-alpha.0)

@jgornick
Copy link
Contributor

@dannyhw Thank you for the update. I understand how frustrating it can be working with workspaces and RN.

What are the hooks issue you're running into? I remember having to deal with something like this as well.

Again, thank you!

@dannyhw
Copy link
Member Author

dannyhw commented Jan 19, 2021

@jgornick basically it's this issue: https://reactjs.org/warnings/invalid-hook-call-warning.html
It's definitely related to the workspace/metro/symlinks because storybook will work with hooks when you install it in your project.

I've tried all kinds of things to make it work but it just takes so long because I need to re-install all the dependencies every time. Otherwise its hard to tell if I'm having issues with cache or symlinking.

I was thinking that I could just remove react from the root package.json or at least make every react dep be the same version. However whenever I do this it either doesn't work or I get typescript errors from emotion and I can't figure out why. The types from emotion in v10 are very confusing and they were improved in v11 but v11 requires hooks as far as I can tell.

All this to say I still don't really know definitively what the issue is, the repo is in need of a big clean out honestly and maybe its just time to suck it up and re-do the tooling setup entirely, unfortunately this is a big task.

@dannyhw
Copy link
Member Author

dannyhw commented Jan 21, 2021

Hey guys, a promising update today.

Putting the tooling issue aside and looking at some core features that 6.0 needs. Had a pair programming session today and
I was able to get a simple CSF story working with help from Norbert. This is a great step because it opens the door for args and controls and so on.

:)

@pachuka
Copy link

pachuka commented Mar 27, 2021

@dannyhw - thanks for your hard work on all of this! I think I may have a solution to your hooks issue (maybe?). So I ran into this in a mono repo project built with lerna that's structured like below:

  • root
    • packages
      • native
        • storybook-app
      • icons
      • web

In my scenario the native package has a peer dependency on the icons package, and then the storybook-app depends on the native and icons package.

Both my native + icons package inevitably do have the react-native package in them, so when I link into the storybook-app I get the same invalid hook issue.

To fix this I had to modify my metro.config.js within the storybook-app to blacklist/exclude the node_modules for those other packages:

/**
 * Metro configuration for React Native
 * https://github.com/facebook/react-native
 *
 * @format
 */
const path = require('path');
const exclusionList = require('metro-config/src/defaults/exclusionList');

// these paths are specific to my structure
const nativeLib = path.resolve(__dirname, '..');
const iconsLib = path.resolve(__dirname, '..', '..', 'icons');

// allow storybook-app to resolve the icons/native packages (similar to symlinking)
const extraNodeModules = {
  'icons': path.resolve(__dirname, '../../icons'),
  'native': path.resolve(__dirname, '../')
};

module.exports = {
  transformer: {
    getTransformOptions: async () => ({
      transform: {
        experimentalImportSupport: false,
        inlineRequires: true
      }
    })
  },
  resolver: {
    // explicitly blacklist the `node_modules` from those other packages
    blacklistRE: exclusionList([
      new RegExp(`${nativeLib}/node_modules/.*`),
      new RegExp(`${iconsLib}/node_modules/.*`)
    ]),
    extraNodeModules: new Proxy(extraNodeModules, {
      get: (target, name) =>
        //redirects dependencies referenced from extraNodeModules/ to local node_modules
        name in target ? target[name] : path.join(process.cwd(), `node_modules/${name}`)
    })
  },
  watchFolders: [path.resolve(__dirname, 'node_modules'), nativeLib, iconsLib]
};

This solved the problem consistently for me.

Hope that helps!

@dannyhw
Copy link
Member Author

dannyhw commented Mar 27, 2021

@pachuka thanks for this, I'll try it out 🙏.

@dannyhw
Copy link
Member Author

dannyhw commented Apr 14, 2021

Firstly I'd like to apologise if you've been waiting for updates on this, covid lockdown has really hit my motivation and some personal stuff has made it hard to make progress. I'm hoping that by breaking down this into smaller parts I can both enable more collaboration and make it easier for me to eat away at the remaining work here.

So back to the 6.0 topic:
This PR has become a bit daunting for me and I think it would be better to split this up into smaller parts. I'm probably going to keep this PR open for now but I want to start opening smaller PR's and issues that target this branch with some smaller additions.

Any one who wants to pick up one of these and give it a go is more than welcome and I will offer as much help I can.

Please note this is not by any means a complete list of features it's more so a todo list for the near future.

I'm splitting this up as follows:

  • Main PR and branch used as a target for changes related for phase 1 of 6.0 which would basically be a working version of 6.0. Once this is done a "next" branch will be created and likely a "next" version on npm for early testing.

Tasks and sub tasks (roughly in priority order):

  • Tooling rework for the mono-repo (this repo), a do-over for the mono-repo with only what is 100% necessary
  • Update the examples
    • examples should share the code for stories/components
    • examples should be more useful/realistic
    • react native components used in storybook could be developed here
    • update to latest version of expo and react native
  • Full CSF support (basic support is done but isn't flexible and doesn't cover edge cases)
  • Args
  • Automatic story detection (partially complete)
    • watcher
    • pre-script
    • make it work like a node binary similar to how you run the server where the script is available just by having storybook installed
  • storybook server (still undecided on the direction this will follow)
    • re-introduce the server in a way that doesn't restrict development of 6.0 (addon maybe?)

Anything you want to discuss feel free to leave a comment or find me in the storybook discord in the react native channel

@jgornick
Copy link
Contributor

jgornick commented Apr 15, 2021

Firstly I'd like to apologise if you've been waiting for updates on this, covid lockdown has really hit my motivation and some personal stuff has made it hard to make progress

@dannyhw No need to apologize ever for open source work, especially during a pandemic! We're all going through it together.

You're doing awesome work, so thank you 😄

@dannyhw dannyhw changed the base branch from master to next-6.0 May 11, 2021 18:36
@dannyhw dannyhw marked this pull request as ready for review May 11, 2021 18:43
@dannyhw
Copy link
Member Author

dannyhw commented May 11, 2021

I will be merging these changes into a new branch called next-6.0.

Going forward all 6.0 changes will be made as smaller PR's into this branch.

I've been using this PR as a way to update everyone on progress however I'd as I mentioned in the previous comment I want to split things up into smaller tasks. From now on I will move that discussion over to the issue #131 and I will pin that issue for discoverability.

Big thanks to @oblador who has recently helped out with the project. Turns out a lot of the issues I was having were much simpler than I was seeing 😅 . Nothing like another perspective to get through a rough patch!

I'll be trying to manage the tasks for 6.0 in the project here

@dannyhw dannyhw merged commit be2e3cf into next-6.0 May 11, 2021
@dannyhw dannyhw changed the title tech: upgrade to 6.0 tech: upgrade to 6.0 (part 1) May 11, 2021
@dannyhw dannyhw mentioned this pull request May 11, 2021
11 tasks
dannyhw added a commit that referenced this pull request May 11, 2021
* tech: remove addon based code from example.

* tech: update versions to 6-0-28

* fix: typescript errors

* fix initial runtime

* worklog

* get things working!

* feat: add expo web example

* native fixes

* fix: expo example

* fix: android expo padding

* worklog 23/11

* add fresh native example

* preval experiments

* remove native-ts

* remove commented stuff

* working pre script

* worklog 1/12/2020

* add requires file to ignore

* refactor: lots of name changes and minor fixes

* storylist experiment

* more storylist experiments

* add simple file watcher on main.js

* add watcher command to example

* first csf story!

* fix: commit file name case change

* fix: commit file name case change #2
@dannyhw dannyhw deleted the tech/upgrade-to-6 branch July 2, 2021 12:21
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.

None yet

4 participants