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 spike remove lerna #135

Merged
merged 13 commits into from
Apr 8, 2019

Conversation

pietrop
Copy link
Contributor

@pietrop pietrop commented Apr 8, 2019

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

This PR #130 in particular addressing this comment


Describe what the PR does

  • removed Create React App(CRA) scripts as storybook replaces it
  • got storybook to work - npm start or npm run storybook
  • added webpack config for storybook to support css modules - See below for more details.
  • got linting to work, by explicitly installing eslint + dependencies vs previously CRA had those installed inside
  • removed .stylelintrc in favour of .eslintrc?
  • got tests to work, via jest - npm run test, and npm run test:ci for travis CI, and npm run test:watch for development
  • figured out how to create a bundle for distribution using "custom" webpack webpack.config.js - see more info below
  • made another test CRA and imported TimedTextEditor only
    • with import TimedTextEditor from '@bbc/react-transcript-editor/TimedTextEditor';
    • and import { TimedTextEditor } from '@bbc/react-transcript-editor'; - see below for more details and reasoning behind the need for this.
    • exposed other components to be imported standalone - see README for details
    • exposed other node modules to be imported standalone - see README for details
  • update README
  • increase version number to be ready to test package via npm

Todo after merge to master

  • publish to npm - npm run publish:public
  • publish storybook to github pages npm run deploy:ghpages
  • css is bundled inside the js code via style-loader and css-loader webpack loaders. However it should be possible to bundle it separately, but couldn't get this to work.

Storybook css modules

  • Storybook does not have support for css modules out of the box.
  • If you remove CRA scripts then you get this issue where css modules don't load in storybook.
  • storybook/webpack.config.js augments the storybook one with support for css modules
  • In previous setup, because CRA was present, with it's webpack config, storybook was showing css modules, as it was augmenting its webpack config with the CRA one.

webpack

Webpack is now used for bundling the component for npm distribution.
Biggest change is that the build script now uses webpack instead of simply using babel

- "build:component": "rimraf dist && NODE_ENV=production babel  packages/ --out-dir dist --copy-files && rimraf dist/**/*.sample.json dist/**/*.sample.js dist/**/example-usage.js dist/**/*.test.js dist/**/*__tests__ dist/**/*__snapshots__ dist/**/*.spec.js",
+ "build:component": "webpack --config webpack.config.js",

see webpack.config.js for more details, and these two blog posts for background.

import only certain components

Using Webpack code splitting entry points as well as exporting from packages/index.js for max flexibility.

As explained in the README you can now do both

import TimedTextEditor from '@bbc/react-transcript-editor/TimedTextEditor';
import { TimedTextEditor } from '@bbc/react-transcript-editor';

The difference is that

However if you are not using TranscriptEditor it is recommended to follow the second option and import individual components like: @bbc/react-transcript-editor/TimedTextEditor 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. (Similarly to how react-bootstrap works)

At the moment webpack complains that some files are above the recommended size limit. It be interesting to see if there's a way to reduce this with some further optimization eg lazyloading etc.. but I'd leave it for a subsequent PR for now

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets: 
  TimedTextEditor.js (271 KiB)
  TranscriptEditor.js (455 KiB)
  index.js (455 KiB)

Excluding tests and sample json

webpack works out the dependency tree from the entry points as it's making it's own dependency graph, and because sample and tests files are not used or imported in production, then they are be automatically excluded in the bundle process.

Jest config

To run jest across the react components, we have to stub the css file dependencies using moduleNameMapper under the jest attribute in package.json

Other things to think about

This could be done as separate repo, but if it's no longer a "mono repo" (in the sense of using lerna, workspaces etc.. where each package is published independently to npm) then some of the naming could be changed as follows

  • perhaps the packages folder could be renamed to lib
  • and module names named could be pascal case following the React components naming convention, instead of the kebab-case required by lerna.
  • requires changing in package.json (main, module, and files attributes as well as the various scripts), webpack.config.js, .storybook/config.js as well as various components imports etc.. across the repo.

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

Ready for review, cc @jamesdools


Additional context

  • It be good to add an ADR capturing some of these things.

Pietro Passarelli - News Labs added 7 commits April 5, 2019 21:33
tests not working yet
using jest, and removed CRA dependency, updated babel config for babel 7 and stubbed css files for jest tests for css node modules
CRA had it's own linter internally, so added linting + dependencies
see PR description for more details
@pietrop pietrop requested a review from jamesdools April 8, 2019 09:05
@pietrop pietrop marked this pull request as ready for review April 8, 2019 09:16
@pietrop pietrop changed the title Monorepo refactor spike remove lerna "Monorepo" refactor spike remove lerna Apr 8, 2019
Copy link
Contributor

@jamesdools jamesdools left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(didn't know where to comment this):

removed .stylelintrc in favour of .eslintrc?

Bring this back - .stylelintrc is a tool to lint .css files - .eslintrc doesn't deal with those which is why we have both files


I'll check this out properly and review on my machine also.

.storybook/config.js Outdated Show resolved Hide resolved
demo/app.js Show resolved Hide resolved
@jamesdools
Copy link
Contributor

Could we also add in an npm script for the demo page?

@pietrop
Copy link
Contributor Author

pietrop commented Apr 8, 2019

Thanks @jamesdools , re npm script for the demo page, what would you like the script to do?

at the moment the demo page shows up in the storybook (which will be published to our github pages and replace current demo app)

But there's also a way to get a direct link to the demo app as a full screen page via storybook (which is then similar to the current/previous demo app page) eg as shown in the README:

[You can see a demo by clicking here ](https://bbc.github.io/react-transcript-editor/iframe.html?id=demo--default)

link wouldn't work yet, coz we haven't published storybook to github pages yet

@pietrop
Copy link
Contributor Author

pietrop commented Apr 8, 2019

Bringing back .stylelintrc I guess I should have been able to tell by the file name

@pietrop pietrop requested a review from jamesdools April 8, 2019 13:24
 playtime on display is double of actual total playtime
 in media player to stop moving while playing
@jamesdools
Copy link
Contributor

Happy for you to squash + merge later - any reason why the travis CI isn't working?

@jamesdools jamesdools merged commit 12d9982 into monorepo-refactor Apr 8, 2019
@jamesdools jamesdools deleted the monorepo-refactor-spike-remove-lerna branch April 8, 2019 15:56
pietrop added a commit that referenced this pull request Apr 8, 2019
* Task: added lerna

* WIP: storybook conversion

* mend

* WIP: adding MediaPlayer

* move TimedTextEditor, TranscriptEditor and adapters util to packages (#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

* Changed repo packages folder structure (#129)

* cleaned up adapters

* changed folder structure

* fixed timecode converter duplice module

* made all packages private except for TranscriptEditor

* working

* "Monorepo" refactor spike remove lerna (#135)

* 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

* Feature: Added custom css loading to storybook (#136)

* Resolved conflict iwth AWS adapter
@pietrop
Copy link
Contributor Author

pietrop commented Apr 9, 2019

making a note that to push to npm keeping import TimedTextEditor from '@bbc/react-transcript-editor/TimedTextEditor'; had to modify package.json so that

  • the bundles are in the dist folder
  • package.json from root is copied into the dist folder
  • npm publish the dist folder

Doing it this way, the dist folder is not published, but rather it's content is published, allowing for the "name spacing"

"main": "index.js",
...
"scripts": {
  ...
  "publish:public": "npm run build:component && rm ./dist/package.json || true && cp package.json ./dist/package.json && npm publish dist --access public",
  ...

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