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

Migrate asset pipeline to Webpack, use TS & React #95

Merged
merged 8 commits into from
Jun 4, 2020

Conversation

eessex
Copy link
Contributor

@eessex eessex commented Jun 4, 2020

Adds TS/React front end toolchain:

  • Installs webpacker gem and compile JS assets via Webpack
  • Addstypescript, react, @artsy/palette and styled-components
  • Adds linting/prettier and pre-commit checks
  • Adds navbar component

Screen Shot 2020-06-04 at 3 21 44 PM

@eessex eessex requested a review from joeyAghion June 4, 2020 19:24
@eessex eessex requested review from jpotts244 and damassi June 4, 2020 19:25
babel.config.js Outdated Show resolved Hide resolved
config/webpacker.yml Outdated Show resolved Hide resolved
config/webpacker.yml Outdated Show resolved Hide resolved
config/webpacker.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

Couple minor issues, but nice one @eessex 👍 I see there are plans on the horizon... for horizon

Dockerfile Outdated Show resolved Hide resolved
consumer.subscriptions.create(
{
channel: "ProjectChannel",
organization_id: $("#organization_subscription_identifier").val(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

If jquery isn't cool and you prefer a more modern/react-friendly alternative, this might be its only usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oo good call - going to make a follow-up task to remove jQuery and update this logic.

@damassi
Copy link
Member

damassi commented Jun 4, 2020

@jonallured - not sure what volts setup looks like, but at the min we should turn on hmr and pretty

Copy link
Member

@jonallured jonallured left a comment

Choose a reason for hiding this comment

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

Whoa, nice one!! I don't have much to add here other than props for this heroic work. 🏆

Oh I do have one suggestion: by convention the default rake task on a Rails project runs all the linters/formatters/tests so I setup Convection like so:

https://github.com/artsy/convection/blob/master/Rakefile#L34

And then Circle just runs that same task:

https://github.com/artsy/convection/blob/master/hokusai/ci.sh#L29

I think this is handy because it means while deving I can run a bundle exec rake and if it's green then I can reliably predict that CI will be green too. I realize that some of this is unnecessary for those that use VS Code and git hooks, but I think it's worth it to set this up so as to be friendly to other setups. What do you think?

@eessex
Copy link
Contributor Author

eessex commented Jun 4, 2020

@jonallured excellent suggestion! I've added a default rake task and am running prettier there now.

"dependencies": {}
"scripts": {
"lint": "eslint ./app --ext ts,tsx,js,jsx --fix",
"prettier": "prettier --write 'app/**/*.(ts|tsx|js|jsx)'",
Copy link
Member

Choose a reason for hiding this comment

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

I think you'd want an additional script like this:

https://github.com/artsy/convection/blob/master/package.json#L6

This ensures that the rake task will fail if there are prettier violations - otherwise the rake task will write those files and happily return 0!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will follow up to add this & rubocop

@eessex eessex merged commit f0c340c into artsy:master Jun 4, 2020
@artsy-peril artsy-peril bot mentioned this pull request Jun 4, 2020
@eessex eessex mentioned this pull request Jun 11, 2020
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.

4 participants