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

Infrastructure upgrade #449

Merged
merged 1 commit into from Mar 25, 2015
Merged

Infrastructure upgrade #449

merged 1 commit into from Mar 25, 2015

Conversation

mtscout6
Copy link
Member

A lot has happened since this projects inception and newer tools are available that can facilitate a better development environment. I have started some work to give the project structure a facelift to meet the current trends of the broader React community. My goals:

  • Source cleanup
    • Rename all js files to *.js
    • Use Babel for our es6 -> es5 transpilation
    • Update all js to es6 standards
      • src
      • test
      • docs
      • doc examples
      • ie8 visual test
    • Use Flow for static type checking?? (Striking for now, can be another PR)
    • ESLint all .js files
  • Build
    • Use Webpack - This should provide more control on how we build our pre-concatenated and minified releases.
    • Drop Grunt, use npm scripts and shell scripts where needed.
    • Promise based release process (faster)
    • Update README to explain how to run.
  • Test
    • Single karma config file
    • Not dependent on building src to disk to run, use karma-webpack which will build the src in memory.
    • Better test watcher that re-runs tests when changing src files. Current watcher only runs if test files change.
    • Fail on linting errors
    • Pull es5-shim from npm
  • Docs
    • Pull bootstrap from npm
    • Pull codemirror from npm
    • Use in browser Babel transformer for doc examples
    • Consolidate package.json deps and scripts to repo root. Leads to less confusion on how to run the docs locally.
    • Change router to react-router, which tends to be up to date with React quicker. (Taken care of in 0.13 Compatibility and Migration #424)
    • Update README for local setup.

This is still a work in progress so I'm open to suggestions for improvement. I am planning to continually rebase and force push this branch, until it's complete.

@mtscout6 mtscout6 force-pushed the infrastructure branch 4 times, most recently from 19a9e0b to e57b7a4 Compare March 23, 2015 20:25
@mtscout6 mtscout6 force-pushed the infrastructure branch 2 times, most recently from 90ad87b to 72d2b2d Compare March 25, 2015 03:12
@mtscout6 mtscout6 changed the title WIP: Infrastructure upgrade Infrastructure upgrade Mar 25, 2015
@mtscout6
Copy link
Member Author

I'm ready to declare success on this. Does anyone see anything glaring that should be addressed before I bring this into master? I'd like to bring this in by end of day tomorrow, baring any revelations by the community.

/cc @joemcbride @pieterv @stevoland @trentgrover-wf

@mtscout6 mtscout6 force-pushed the infrastructure branch 2 times, most recently from cc97728 to df23269 Compare March 25, 2015 14:15
@joemcbride
Copy link
Member

Really glad to see this update. 👍

I pulled down the source and have been doing some smoke testing. I'm seeing some funky styling /scrolling issues with CodeMirror. I also ran into an issue with eslint not finding a rule (I have latest eslint). Double checking that its not a local issue.

@joemcbride
Copy link
Member

So I cleared out all my local npm modules and re-ran npm install. All unit tests pass.

Eslint dies on: Error: Definition for rule 'no-global-strict' was not found.

This is what I'm seeing in the docs in Chrome/Safari/Firefox:
screen shot 2015-03-25 at 11 21 33 am

The re-compile of code works fine, just funky styling. Do you see that styling issue locally?

.cm-s-solarized .cm-comment { color: #93a1a1; }

.CodeMirror {
height: auto;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this line is the cause of the extra-tall editor height, through removing it causes other issues (scrolbars).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, we fixed this on the last pull request that came in and I forgot to apply it here. I need to do this: https://github.com/react-bootstrap/react-bootstrap/blob/master/docs/assets/style.css#L71-L73. Thanks for the find, I'll update that.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@mtscout6
Copy link
Member Author

I ran into that eslint issue as well at one point. I made sure that everything was up to date and it went away. Is it likely that one of your global eslint installs is causing problems? Do you see it when you run npm run lint?

@joemcbride
Copy link
Member

I may not have the global react plugin mac side. Checking that.

@mtscout6
Copy link
Member Author

It should just work if you do it through npm run lint is that the case for you?

@joemcbride
Copy link
Member

No, I get the error using npm run lint.

@joemcbride
Copy link
Member

$ npm run lint

> react-bootstrap@0.18.0 lint /Users/jomc/Dev/rb-es6
> eslint src test docs ie8 tools webpack karma.conf.js webpack.config.js webpack.docs.js

/Users/jomc/Dev/rb-es6/node_modules/eslint/lib/eslint.js:657
                    throw new Error("Definition for rule '" + key + "' was not
                          ^
Error: Definition for rule 'no-global-strict' was not found.
    at /Users/jomc/Dev/rb-es6/node_modules/eslint/lib/eslint.js:657:27
    at Array.forEach (native)
    at EventEmitter.module.exports.api.verify (/Users/jomc/Dev/rb-es6/node_modules/eslint/lib/eslint.js:630:16)
    at processFile (/Users/jomc/Dev/rb-es6/node_modules/eslint/lib/cli-engine.js:193:27)
    at /Users/jomc/Dev/rb-es6/node_modules/eslint/lib/cli-engine.js:293:26
    at /Users/jomc/Dev/rb-es6/node_modules/eslint/lib/util/traverse.js:61:17
    at Array.forEach (native)
    at traverse (/Users/jomc/Dev/rb-es6/node_modules/eslint/lib/util/traverse.js:41:54)
    at /Users/jomc/Dev/rb-es6/node_modules/eslint/lib/util/traverse.js:63:17
    at Array.forEach (native)

@joemcbride
Copy link
Member

Since CI passes, I can only guess this is a local issue for me. I would be fine with you pulling this in while I figure this out.

A lot has happened since this projects inception and newer tools are
available that can facilitate a better development environment. This
work gives the project structure a facelift to meet the current trends
of the broader React community.

- Source cleanup
  - Rename all js files to *.js
  - Use [Babel](http://babeljs.io/) for our es6 -> es5 transpilation
  - Update all js to es6 standards
    - src
    - test
    - docs
    - doc examples
    - ie8 visual test
  - [ESLint](http://eslint.org/) all .js files
- Build
  - Use [Webpack](http://webpack.github.io/) - This should provide more
    control on how we build our pre-concatenated and minified releases.
  - Drop Grunt, use npm scripts and shell scripts where needed.
  - Promise based release process (faster)
  - Update README to explain how to run.
- Test
  - Single karma config file
  - Not dependent on building src to disk to run, use
    [karma-webpack](https://github.com/webpack/karma-webpack) which will
    build the src in memory.
  - Better test watcher that re-runs tests when changing src files.
    Current watcher only runs if test files change.
  - Fail on linting errors
  - Pull `es5-shim` from npm
- Docs
  - Pull `bootstrap` from npm
  - Pull `codemirror` from npm
  - Use in browser Babel transformer for doc examples
  - Consolidate package.json deps and scripts to repo root. Leads to
    less confusion on how to run the docs locally.
  - Update README for local setup.
@mtscout6
Copy link
Member Author

Turns out the eslint problem for @joemcbride was with the docs/node_modules folder. For those of you that have had this repo cloned before it would be a good idea to run git clean -dfx after this PR is merged and re-run npm install. If that does not completely clear your node_modules and docs/node_modules folders then you will need to remove them manually.

mtscout6 added a commit that referenced this pull request Mar 25, 2015
@mtscout6 mtscout6 merged commit 24a98da into master Mar 25, 2015
@mtscout6 mtscout6 deleted the infrastructure branch March 25, 2015 21:36
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

2 participants