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

Monorepo refactor #130

Merged
merged 11 commits into from
Apr 8, 2019
Merged

Monorepo refactor #130

merged 11 commits into from
Apr 8, 2019

Conversation

pietrop
Copy link
Contributor

@pietrop pietrop commented Apr 5, 2019

Is your Pull Request request related to another issue in this repository ?

#122

Describe what the PR does
Restructures the codebase to be more modular, introducing storybook

State whether the PR is ready for review or whether it needs extra work
still in draft

jamesdools and others added 6 commits April 4, 2019 16:11
…128)

* moved TimedTextEditor and TranscriptEditor to packages

also created stories, and package.json for each, but can't test them in storybook coz they have dependencies on adapters in Util folder

* moved Util and demdemo app

* got storybook working

* added demo app to storybook

* mend

* Fix: commenting out demo
* cleaned up adapters

* changed folder structure

* fixed timecode converter duplice module

* made all packages private except for TranscriptEditor
@pietrop
Copy link
Contributor Author

pietrop commented Apr 5, 2019

I am sold on the storybook, but some things to figure out are, do we really need Lerna?

Argument for removing Lerna in the monorepo setup.

  1. npm7 is likely to have workspaces
    Not only npm latest version is head to head with yarn features, but also npm@7, which is predicted for late summer and fall of 2018 is very likely to have Workspaces
    imho lerna is poorly documented, and hard to get up and running with.
    using lerna would also require rest of our team to wrap their heads around that for this specific project, has its not currently part of our stack, and might limit the number of contributions etc.
    It's tempting and common to add a lot of tooling when working with node/js, but most importantly tho, it seems to solve a problem that we don't have for this specific project. Lerna is for managing monorpeos where you publish to npm separate packages to be installed as standalone.
    Eg In our repo we are leaning torwards publishing one package @bbc/react-transcript-editor and letting you import the parts that are needed for your project.
    Which leads me to my second point below.

  2. It's possible export various components from the same npm module
    With React Bootstrap for example
    When you install react-bootstrap with npm, if for example you only want a button. doing import Button from 'react-bootstrap/Button'; allows you to only bundle the btn and not the rest of the react-bootstrap library.

You should import individual components like: react-bootstrap/Button rather than the entire library. Doing so pulls in only the specific components that you use, which can significantly reduce the amount of code you end up sending to the client.

  1. Lerna requires extra config needed for simple things
    Seems also that Lerna requires extra config for simple things, such as tests. couldn't get the test to work, altho found this article sort of of useful, as tests would need transpiling and jest couldn't find node_modules etc..

Will do a separate spike from this branch/PR to show what it would look like without lerna.

@pietrop pietrop mentioned this pull request Apr 8, 2019
21 tasks
* got storybook working

tests not working yet

* fixed tests

using jest, and removed CRA dependency, updated babel config for babel 7 and stubbed css files for jest tests for css node modules

* Added support for demo app in storybook

* fixed eslint

CRA had it's own linter internally, so added linting + dependencies

* cleaned up export scripts in package.json

* updated README

* finalised refactor

see PR description for more details

* rename demo app editor to demoTranscript

* bringing back style lint, and fixing lint in storybook config

* updated with current master AWS adapter

* linting

* fix #132 playtime displaied double

 playtime on display is double of actual total playtime

* temporary fix #73 monospace duration and current time

 in media player to stop moving while playing
@pietrop pietrop marked this pull request as ready for review April 8, 2019 17:39
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