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

Feature: Introduce Storybook + monorepo structure #122

Closed

Conversation

chrishutchinson
Copy link

Is your Pull Request request related to another issue in this repository ?
Initially raised in issue #109.

Describe what the PR does
This PR overhauls the structure of the application, otherwise keeping everything else intact. The reasons for this change are to support more modular use of the components for users who wish to bring their own features and UI, while taking advantage of the great work inside this codebase.

A fairly comprehensive list of changes is below:

  • Move most components, utilites, adapters and exporters into logical top-level packages (some contain sub-components, where there is no need for them to be shared)
  • Introduce Lerna for managing the monorepo, alongside Yarn Workspaces which is required for it to run (npm may not work..)
  • Introduce Storybook, used primarily for exposing the new React packages in a local development environment (although it also creates a nice static site for demoing!)
  • Run prettier on all JavaScript files, and simplifies the eslint config so there are not conflicts with prettiers defaults
  • Scopes all packages under the @bbc-transcript-editor/* namespace, which would need to become a new organisation in NPM

State whether the PR is ready for review or whether it needs extra work
This PR is absolutely ready for review, but also very possibly will need further work. That entirely depends 😃

Additional context
I believe there will be some additional work in this PR, specifically:

  • Agreement, then BBC approval, of transitioning to a new NPM organisation (@bbc-transcript-editor/*) for the packages
  • Extensive testing, to make sure I haven't broken any specific logic while moving things around
  • Finalise changes to the README
  • Agreement on the terms for adapters and exporters (still not sure on this)
  • Updating the travis config to support automated packaging and deployment to NPM on merge to master
  • Determine how to bundle + ship each package to NPM, as an ES6 module, or as a transpiled ES5 component

One this PR has been merged, there may be some additional work, notably:

  • Introducing a new "wrapper" component to centrally manage analytics events, and registering which adapters + exporters to use (useful for those that want to create custom interfaces using some of these components)

Screen Shot 2019-03-19 at 20 31 37

Screen Shot 2019-03-19 at 20 31 46

Screen Shot 2019-03-19 at 20 31 48

Screen Shot 2019-03-19 at 20 31 52

Screen Shot 2019-03-19 at 20 31 54

Screen Shot 2019-03-19 at 20 31 57

@pietrop
Copy link
Contributor

pietrop commented Mar 19, 2019

Thanks @chrishutchinson , will try to have a look with @jamesdools by the end of the week.

Meanwhile travis CI it gives this error npm ERR! missing script: test-ci.

It seems like we might need to rethink the .travis.yml settings for the monorepo?

Perhaps instrad of npm run test-ci at.travis.yml#L12 there might be the equivalent yarn workspaces command to run tests eg yarn workspaces run test --ci?

@chrishutchinson
Copy link
Author

chrishutchinson commented Mar 19, 2019

Thanks @pietrop - looks like huge number of lines of code have changed, although I think most of that is the sample JS/JSON files 😅

Re. the Travis config, it'll definitely need some work. It'd be good to understand what your CI-specific test suite was doing, as it may be possible to just run the regular suite (i.e. yarn test without a --ci flag). Jest does have a --ci flag, although it's purely used for snapshot testing, which I don't think we have any of.

Let me know, I can push a change to travis.yml to test that out if you think it's worthwhile!

@pietrop
Copy link
Contributor

pietrop commented Mar 20, 2019

Yeah, push a .travis.yml with the yarn workspaces run test --ci on line 12.

If I got this right, this should run the test scripts in the packages modules. Worth seeing if it works as is, or if it needs tweaking

@pietrop
Copy link
Contributor

pietrop commented Mar 20, 2019

didn't work

I think we also need to install yarn on travis and we might not need npm?

Try this .travis.yml

language: node_js
node_js:
 - "10"

before_install:
  - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version 1.15.2
  - export PATH=$HOME/.yarn/bin:$PATH
cache:
  yarn: true

install:
  - node --version
  - yarn install
  - yarn workspaces info

script:
  - yarn workspaces run test --ci 

also note

A YAML file use spaces as indentation, you can use 2 or 4 spaces for indentation, but no tabs (more here)

@pietrop
Copy link
Contributor

pietrop commented Mar 20, 2019

As a side note, might be good to get these PR #114 , #93, #121 and possibly #60 in before the re-org it might be a bit more tricky to merge afterwards

@pietrop
Copy link
Contributor

pietrop commented Mar 20, 2019

if the last one doesn't work, another travis.yml option is explained in this article

@chrishutchinson
Copy link
Author

Weird output in Travis:

Now using node v10.15.3 (npm v6.4.1)
$ node --version
v10.15.3
$ npm --version
6.4.1
$ nvm --version
0.34.0
$ yarn --version
1.3.2
4.59s$ yarn
yarn install v1.3.2
(node:4326) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
error An unexpected error occurred: "patterns.map is not a function".
info If you think this is a bug, please open a bug report with the information provided in "/home/travis/build/bbc/react-transcript-editor/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

The command "eval yarn " failed. Retrying, 2 of 3.

Absolutely no idea why yarn would fail to install...

@pietrop
Copy link
Contributor

pietrop commented Mar 20, 2019

this thread might help?
TL;DR: might be worth updating to latest yarn release in travis script?

@pietrop
Copy link
Contributor

pietrop commented Mar 21, 2019

Having a look at this today, I feel like this could also do with having an ADR, and updated README, to flesh out a bit more the intentions and workings at high level etc..

@pietrop
Copy link
Contributor

pietrop commented Mar 21, 2019

I'd be in favour of a less flat folder structure for the packages.
currently

├── packages
│   ├── adapter-amazon-transcribe
│   ├── adapter-autoedit-2
│   ├── adapter-bbc-kaldi
│   ├── adapter-speechmatics
│   ├── core
│   ├── editor
│   ├── eslint-config
│   ├── exporter-text
│   ├── icons
│   ├── keyboard-shortcuts
│   ├── media-player
│   ├── settings
│   ├── style-guide
│   ├── timed-text-editor
│   ├── util-create-entity-map
│   ├── util-generate-entities-ranges
│   ├── util-timecode-converter
│   └── video-player

It could be:

├─── demo
└─── packages
	├── stt-adapters
	│	├── create-entity-map
	│	├── generate-entities-ranges
	│	├── stt-bbc-kaldi-to-draftjs
	│	├── stt-speechmatics-to-draftjs
	│	├── stt-aws-to-draftjs
	│	├── autoedit2-to-draftjs
	│	└── stt-ibm-to-draftjs
	├─── util
	│	└── timecode-converter
	├─── exporters
	│	├── draftjs-to-text
	│	└── ...
	│
	├── components
   	│	├── transcript-editor
   	│	├── timed-text-editor 
   	│	├── media-player
   	│	├── video-player
   	│	├── settings
   	│	└── keyboard-shortcuts
	│
	└── config
   		├── core  // <-- what is in core?
   		├── eslint-config
   		├── icons
    		└── style-guide

Thinking of names for the individual modules that would make sense if it's published as standalone.

The components could also be at root level (or could follow a more atomic design approach eg with atoms, molecules and organism - but this could be at a much later stage if needed/useful)no strong opinion on that at this point, can always changed later.

@pietrop
Copy link
Contributor

pietrop commented Mar 21, 2019

Could we also add the addon-info, knobs and jest to the story book?

@pietrop pietrop mentioned this pull request Mar 22, 2019
@pietrop pietrop changed the base branch from master to monorepo April 4, 2019 14:31
@pietrop pietrop changed the base branch from monorepo to master April 4, 2019 15:08
@pietrop
Copy link
Contributor

pietrop commented Apr 9, 2019

@chrishutchinson thanks for this! has been very helpful to look at and use as a guide.
closing in favour of PR #130
Any thoughts or questions, feel free to reach out :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants