Skip to content

Toward next Major version #91

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

Merged
merged 31 commits into from
Apr 10, 2017
Merged

Toward next Major version #91

merged 31 commits into from
Apr 10, 2017

Conversation

nickbalestra
Copy link
Collaborator

@nickbalestra nickbalestra commented Apr 7, 2017

As per #85

Goal is to get to a state where we can work together and merge into a single flavor that will allows for both choosing the language (typescript/javascript) and the stream library (xstream, most, rxjs)

Current status

  • webpack2
  • preset-env
  • uglify / minification
  • HtmlWebpackPlugin
  • JSX -> ProvidePlugin({snabb: 'snabbdom-jsx'})
  • inquirer -> for eject confirmation
  • inquirer -> for init (choose js or ts, choose streamlib -> rxjs, xstream, most)
  • move inquirer to create-cycle-app, in case that no flag --flavor is passed, and pass down answers to init
  • adhoc templates for javascript
  • adhoc webpack configs for javascript
  • Work with yarn if yarn detected
  • General cleanup
  • Do not use snabbdom-jsx it has bugs, use https://github.com/Swizz/snabbdom-pragma instead

To be done in follow-up PRs:

@nickbalestra nickbalestra changed the title New scripts [WIP] New scripts Apr 8, 2017
@jvanbruegge
Copy link
Collaborator

jvanbruegge commented Apr 8, 2017

  1. Do not use snabbdom-jsx it has bugs, thanks to a few PRs from me https://github.com/Swizz/snabbdom-pragma is the way to go
  2. Why not use webpack-blocks, it makes config a lot easier (for us and for the people after eject)
  3. I just merged the 2.0.0 version of the one-fits-all flavor, so that might be helpful

@nickbalestra
Copy link
Collaborator Author

nickbalestra commented Apr 8, 2017

Thanks!
I prefer to be a bit closer to the "webpack-metal". So that we don't rely on another project and users don't have to learn another thing. If you eject then you suppose to be able to deal with webpack and is the your issue from that point onward. So I think is better to leave that decision to the user and don't make too opinionated decision for what he'll get after eject, the more metal, the better imho.
In this direction I find the create-react-app way of commenting the webpack configs vert compelling and of high value to an 'ejected-user` (and also for us as we really know what is going on and we have full control).

I'm structuring it in a way that Iside configs and template/src we would have 2 folders, 'javascript' and 'typescript' so that we can mantain ad-hoc configuration and source file for both.

I aim to push something for you to look at today/tomorrow

@@ -1,15 +1,12 @@
<!DOCTYPE html>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not needed when you use index.ejs and the HTMLWebpackPlugin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no need to use an .ejs with the TMLWebpackPlugin, you can just use an html file and inject into it, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure, either way, you have both

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hehe...is . WIP :) not sure i have pushed everything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree that there should be only 1

@@ -0,0 +1,12 @@
module.exports = replacements => `${replacements.import}

export function App (sources) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use the counter example as the base app, like I did here. Has the advantage of showing MVI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Totally agree

@@ -1,7 +1,8 @@
// import assert from 'assert'
module.exports = replacements => `// import assert from 'assert'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Take a look at the new test setup in the one-fits-all flavor

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the tests? Yes those are nice, wi'll defintiely bring/merge all those ins

@jvanbruegge
Copy link
Collaborator

What are the dev-utils for?

@nickbalestra
Copy link
Collaborator Author

nickbalestra commented Apr 8, 2017

I initially forked them from react-dev-utils, but now using them directly, no need to fork them. It provide a nice little sets of utilities, mainly a great webpackHotDevClient with overlay of errors & co. it make the experience super nice! => https://github.com/facebookincubator/create-react-app/tree/master/packages/react-dev-utils

@jvanbruegge
Copy link
Collaborator

jvanbruegge commented Apr 8, 2017

https://github.com/facebookincubator/create-react-app/tree/master/packages/react-dev-utils#webpackhotdevclientjs

It currently supports only Webpack 1.x

This is not good, WP 1 was official deprecated

@jvanbruegge
Copy link
Collaborator

We could also incorporate https://github.com/Widdershin/cycle-restart to make hot reloading cooler

@nickbalestra
Copy link
Collaborator Author

nickbalestra commented Apr 8, 2017

Lol I guess someone should do a PR as they are using [webpack2]...

https://pbs.twimg.com/media/B3qIJLFCcAEJLWm.jpg
(https://github.com/facebookincubator/create-react-app/blob/master/packages/react-scripts/package.json#L57) and work fine (I've also tested it without any issue)

yes that will be awesome. Added to the list!

@nickbalestra
Copy link
Collaborator Author

@jvanbruegge Once I've cleaned up some stuff, I'll ping you so that you can bring in the typeScripts parts and we can then work together on putting it together. Would be great to launch it at the cycleconf

@jvanbruegge
Copy link
Collaborator

sure
indeed

@jvanbruegge
Copy link
Collaborator

Maybe we should also have a third question about test runner, ava seems to be quite hot nowadays. So we could provide tests with mocha-webpack or ava-webpack

@nickbalestra
Copy link
Collaborator Author

nickbalestra commented Apr 8, 2017

Imho test-suite choice is a bit out of scope for this PR. I'll rather keep the same test suite for the whole monorepo (jest atm). We can always add that later in if we believe is a must.

@jvanbruegge
Copy link
Collaborator

does jest work with webpack?

@nickbalestra
Copy link
Collaborator Author

@nickbalestra
Copy link
Collaborator Author

I think I'll brake this PR. Once I got the whole JS part in, we can review it and merge.
Then we can open PRs for

  • moving to counter example
  • adding typescript src
  • adding tests
  • adding css/autoprefixer
  • Widdershin/cycle-restart
  • eslinting/prettier
  • docs

@jvanbruegge
Copy link
Collaborator

Yeah, i think this is better

@nickbalestra
Copy link
Collaborator Author

Cool,
I'll try to have this one ready for tomorrow so that we can divide and conquer this. Exciting times!
We can put all those releases behind a next flag until we hit major

@nickbalestra
Copy link
Collaborator Author

nickbalestra commented Apr 9, 2017

@jvanbruegge I've done everything. Few major things:

  • CLI if no flavor is passed now prompt user for language (javascript, typescript(not yet working as files are missing)) and stream library (xstream, most, rxjs).
  • If flavor is passed, prompting is delegated to flavor, cycle-scripts support both so that you can run it locally or as a fork with no issue.
  • Unique file for configuring strings to interpolate templates and various dependencies (basic, language, and stream-lib related) - /configs/flavor.js
  • It now support yarn (by autodetecting it (pass --noyarn flag if you have yarn but want to force it to use npm instead)
  • On bundling, errors are displayed on the page via an overlay
  • scripts structure now has configs/<language> (typescript to be added), same for template/src/<language>
  • No need for adding babel config to packaje.json anymore of flavor anymore, directly declared in the webpack.configs as we use webpack node.api)
  • and a lot more...

I think this should be a good state to move forward as per our dicussion.
Let me know

@nickbalestra
Copy link
Collaborator Author

Addresses #98 #11 #70(via native template literals)

@nickbalestra nickbalestra changed the title [WIP] New scripts Toward next Major version Apr 9, 2017
@jvanbruegge
Copy link
Collaborator

The points are good, but i have few remarks about the code (only had quick look):

  • the point about the babelrc was to allow changes, eg for the env-preset if you cant di this if its hidden away.
  • using the webpack node api means you are leaving traces post-eject (most likely a scripts folder), im not really a fan of this. A partial solution would be to copy all config like normal and have a seperate npm pacjage with the scripts. that way you dont have to have them in your git (dont like this either)
  • im not sure if we need seperate js/ts templates. TSC. if you compile ts to es6 you will get your js template for free
    i will take a deeper look tomorrow

@jvanbruegge
Copy link
Collaborator

  • There is a lot of duplication in the webpack configs, thats why i like webpack-blocks. Even without i would refactor at least to a common file and seperate prod and dev files

@nickbalestra
Copy link
Collaborator Author

nickbalestra commented Apr 10, 2017

I know, and you could also use something like webpack merge. But to be honest, is it that bad to have just two separate plain config files, one for dev and one for prod?

Maybe something we can explore in a separate PR after we bring in typescript and the other things? perhaps bringing out 'paths' in its own file..

If Ok for you I'm going to merge this

@jvanbruegge
Copy link
Collaborator

ok

@jvanbruegge
Copy link
Collaborator

i would wait with the release untul we got a few PRs done

@nickbalestra
Copy link
Collaborator Author

Totally agree (also cause it will brake if you select typescript now :D)!

The only version published will be a minor with scoped core-scripts as per #100.

If we need for testing and try I suggest we'll release the work we are now doing behind a npm flag until we get to a rc state

@jvanbruegge
Copy link
Collaborator

Can you add me to this repo? Then i can assign myself to the tickets and dont have to constantly update my fork

@nickbalestra
Copy link
Collaborator Author

Sure, done 😃

@nickbalestra nickbalestra merged commit 351825a into master Apr 10, 2017
@nickbalestra nickbalestra deleted the new-scripts branch April 10, 2017 08:47
perjerz pushed a commit to perjerz/create-cycle-app that referenced this pull request Nov 12, 2018
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