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

React with ES6 and Webpack Implementation #1813

Closed

Conversation

acamposruiz
Copy link

Hi!

Here I have develop an aproach with React 0.14 and webpack with ES6, I think can be a good thing present a way to modularize this project with webpack.

The project is in examples/react-webpack-es6.

Thanks for suggestions!

Antonio

@mention-bot
Copy link

@acamposruiz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sindresorhus and @passy to be potential reviewers.

@acamposruiz
Copy link
Author

acamposruiz commented Aug 28, 2017

Hi @dhruvdutt , I have cleaned up my commit messages and rebasen as you asked in this new branch and Pull Request

@acamposruiz acamposruiz mentioned this pull request Aug 28, 2017
@dhruvdutt
Copy link

Cool. Are there still issues with jshint in examples/react-webpack-es6/build/app.js?

@acamposruiz
Copy link
Author

Appears that way so I will check all that and fix it as soon as I can

Copy link

@dhruvdutt dhruvdutt left a comment

Choose a reason for hiding this comment

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

You still need to migrate todoModel.js to ES6.
Maintain indentations consistency across all files. (preferably 4 spaces)

@acamposruiz acamposruiz reopened this Aug 30, 2017
Copy link

@dhruvdutt dhruvdutt left a comment

Choose a reason for hiding this comment

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

Why added node_modules?

@acamposruiz
Copy link
Author

I do not know if it is made a npm install on server on every project in order to let the example work out, do you know?

@acamposruiz
Copy link
Author

acamposruiz commented Aug 30, 2017

I have that doubt because of node_modules folder is not included in .gitignore in other examples, why?

@acamposruiz
Copy link
Author

In react example is included node_modules:

captura de pantalla 2017-08-30 a las 13 56 20

@acamposruiz
Copy link
Author

Hi @dhruvdutt , Do you know anything about this error on travis?
(node:9520) [DEP0022] DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead
https://travis-ci.org/tastejs/todomvc/builds/269962365?utm_source=github_status&utm_medium=notification

@dhruvdutt
Copy link

Okay, the other examples have node_modules folder which includes the final dist only and not the src. I believe this must have been done to directly serve package assets through GitHub static hosting.
The current node_modules seems too big to commit. How about we generate a single file app.js including all vendor code and commit it?

Not sure about travis-ci check. Some nested dep seems to cause the issue.

@acamposruiz
Copy link
Author

acamposruiz commented Aug 30, 2017

I've already removed almost all node_modules, only todomvc dependences will remains there. But travis-ci issue keep stay there and it seems like it has been happening for a while, so many others contributions remains unmerge because of this. I really do not know how to deal with this but I will keep digging some days. :)

@acamposruiz acamposruiz reopened this Aug 30, 2017
@FadySamirSadek
Copy link
Collaborator

@acamposruiz Are you still intrested in landing this ?

@acamposruiz
Copy link
Author

@FadySamirSadek yeah I'm still interested ;), Do you want me to do anything? what's next?

@sagirk
Copy link

sagirk commented Jul 11, 2018

@acamposruiz

  1. Branch has conflicts. Needs a rebase.
  2. Build is failing. Something to do with tests in tests/memory.js.

@FadySamirSadek
Copy link
Collaborator

Please refer to #2219

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.

None yet

5 participants