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

Small fixes to achieve reproducible build #661

Merged
merged 3 commits into from
Jan 27, 2017

Conversation

squadette
Copy link
Contributor

@squadette squadette commented Jan 1, 2017

hi,

I'm trying to achieve green tests on my machine. So far I'm unsuccessful, but I've found few issues which I'd like to fix in master to help everyone else.

Thank you,


This change is Reviewable

@coveralls
Copy link

coveralls commented Jan 1, 2017

Coverage Status

Coverage remained the same at 99.294% when pulling 5fbe4a2 on squadette:for-upstream into 13cff3f on shakacode:master.

@coveralls
Copy link

coveralls commented Jan 1, 2017

Coverage Status

Coverage remained the same at 99.294% when pulling 7d0f9e6 on squadette:for-upstream into 13cff3f on shakacode:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.294% when pulling 7d0f9e6 on squadette:for-upstream into 13cff3f on shakacode:master.

@justin808
Copy link
Member

Some suggestions


Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


.gitignore, line 12 at r2 (raw file):

*.gem
/vendor/
*~

This is due to emacs probably and nothing to do with React on Rails. However, I'm OK with this if you think it's better here rather than your global gitignore.


CONTRIBUTING.md, line 89 at r2 (raw file):

When you use a relative path, be sure to run the above `npm install` command whenever you change the node package for react-on-rails.

While we'd prefer to us `npm link`, we get errors. If you can figure out how to get `npm link react-on-rails` to work with this project, please file an issue or PR! This used to work with babel 5.

Since we're getting typos: use not us.


CONTRIBUTING.md, line 144 at r2 (raw file):

    alias: {
      react: path.resolve('./node_modules/react'),
      'react-dom': path.resolve('./node_modules/react-dom'),

Why is this needed?


CONTRIBUTING.md, line 166 at r2 (raw file):

cd <top level>
npm run dummy:spec

correct


lib/generators/react_on_rails/templates/base/base/client/package.json.tt, line 17 at r2 (raw file):

  "cacheDirectories": ["node_modules", "client/node_modules"],
  "dependencies": {
    "babel": "^6.5.2",

why is babel removed? We should be locking the version of it.


spec/dummy/client/package.json, line 12 at r2 (raw file):

  "dependencies": {
    "autoprefixer": "^6.6.0",
    "babel": "^6.5.2",

why is babel removed? We should be locking the version of it.


Comments from Reviewable

@justin808
Copy link
Member

@squadette Any update?

@squadette
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


.gitignore, line 12 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

This is due to emacs probably and nothing to do with React on Rails. However, I'm OK with this if you think it's better here rather than your global gitignore.

I'll setup my global gitignore, makes sense.


CONTRIBUTING.md, line 144 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why is this needed?

I don't know. But In the doc it says:

Side note: It's critical to use the alias section of the webpack config to avoid a double inclusion error. This has already been done for you in the example and dummy apps, but for reference:

But in example and dummy apps it has both react and react-dom, while in .md it only mentions react. I'd also like to understand why exactly we need this special treatment.


lib/generators/react_on_rails/templates/base/base/client/package.json.tt, line 17 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

why is babel removed? We should be locking the version of it.

babel package is deprecated/no-op. It only warns about the need to use babel-cli.

https://github.com/babel/babel/blob/master/packages/babel/cli.js


Comments from Reviewable

@justin808
Copy link
Member

Please rebase on top of master. I'd like to create a release today.

@justin808
Copy link
Member

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 144 at r2 (raw file):

Previously, squadette (Alexey Mahotkin) wrote…

I don't know. But In the doc it says:

Side note: It's critical to use the alias section of the webpack config to avoid a double inclusion error. This has already been done for you in the example and dummy apps, but for reference:

But in example and dummy apps it has both react and react-dom, while in .md it only mentions react. I'd also like to understand why exactly we need this special treatment.

Good catch. I wonder if this is still needed. Safer to do this.


spec/dummy/client/package.json, line 12 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

why is babel removed? We should be locking the version of it.

@squadette Any idea here?


Comments from Reviewable

@squadette
Copy link
Contributor Author

Updated as per discussion.

Thanks,

@coveralls
Copy link

coveralls commented Jan 7, 2017

Coverage Status

Coverage remained the same at 99.298% when pulling 317dc5d on squadette:for-upstream into f929478 on shakacode:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.298% when pulling 317dc5d on squadette:for-upstream into f929478 on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.298% when pulling 317dc5d on squadette:for-upstream into f929478 on shakacode:master.

@@ -14,7 +14,6 @@
},
"cacheDirectories": ["node_modules", "client/node_modules"],
"dependencies": {
"babel": "^6.5.2",
Copy link
Member

Choose a reason for hiding this comment

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

@squadette why is babel removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'babel' is deprecated.

@justin808 justin808 merged commit ea69e3c into shakacode:master Jan 27, 2017
@justin808
Copy link
Member

Thanks @squadette

justin808 pushed a commit that referenced this pull request Feb 11, 2017
* Typo in 'npm run dummy:spec'
* 'babel' package is deprecated, remove from dependencies
* Mention that 'react-dom' also needs to be in webpack.conf; fix typo
justin808 added a commit that referenced this pull request Feb 11, 2017
…e-redux-div-to-json-script

* origin/master: (42 commits)
  Release 6.5.1
  Update CHANGELOG.md
  Ability to work without sprockets (#671)
  Change Turbolinks unmount event from before-visit to before-render (#709)
  Update generator.md
  Update README.md
  Update i18n.md
  Small formatting tweak
  Release 6.5.0
  Updated CHANGELOG.md
  Update README.md
  Update README.md
  Update README.md
  Fix incorrect "this" references of Node.js SSR
  Remove reference to heroku-buildpack-multi (#698)
  Small fixes to achieve reproducible build (#661)
  Update PROJECTS.md
  Doc Changes for links on gitbook
  Update README.md
  Added property renderedHtml to return gen func
  ...
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.

3 participants