-
Notifications
You must be signed in to change notification settings - Fork 5
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
Cycle one-fits-all #1
Conversation
'xstream' | ||
] | ||
|
||
switch (streamLib) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
streamLib options are not supported anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied your ts-webpack one and adjusted it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is everything xstream now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its up to your flavor, that what a flavor is all about.
appPackage.dependencies = appPackage.dependencies || {} | ||
appPackage.devDependencies = appPackage.devDependencies || {} | ||
appPackage.scripts = { | ||
'start': 'NODE_ENV=development webpack --config webpack.config.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you doing here, and I can also see what lead to that. Up till version 3, we were less cleaner with things. for example we were adding dependencies to the package.json while the only dependency that should be there (until you decide to eject) should only be your flavor (that will hide behind all the complexity). By using webpack in this way you leaking into the package json some of the module dependencies...
As a reference check the latest cycle-scripts - This is now the only core-flavor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, I've updated the reference, somehow during v3 release scripts weren't installing basics dependencies as a separate step -> https://github.com/cyclejs-community/create-cycle-app/blob/master/packages/cycle-scripts/scripts/init.js#L8-L12
Should I use a .babelrc file in the template or should I modify the package.json? |
At the moment we are extending the package.json |
d26e487
to
cd26d39
Compare
I think I got everything |
b6520c1
to
8a95635
Compare
@@ -0,0 +1,21 @@ | |||
The MIT License | |||
|
|||
Copyright (c) 2017 Nick Balestra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should put your name here :)
8a95635
to
3921989
Compare
If I'm not completely wrong, the .babelrc file is now read and added to the package.json, so there is no double configuration |
c683b59
to
8b67e3e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! I've tried to run it locally, The eject
script seems to works fine. The others require few minor fixes. To reproduce just install flavor from local: create-cycle-app appName --flavor /absolute/path/to/flavor-directory
} | ||
|
||
const babelrc = JSON.parse(fs.readFileSync( | ||
path.join(__dirname, 'scripts', '.babelrx'), { encoding: 'utf-8' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This throws an error as there is not such file. I guess you meaned something like: path.join(__dirname, 'configs', '.babelrc')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course. Will fix in a minute
const port = 8080 | ||
process.env.NODE_ENV = 'development' | ||
|
||
const config = require('./configs/webpack.config.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to run it and it throws the following errors:
fallbackLoader option has been deprecated - replace with "fallback"
loader option has been deprecated - replace with "use"
fallbackLoader option has been deprecated - replace with "fallback"
loader option has been deprecated - replace with "use"
testApp/node_modules/webpack-merge/lib/join-arrays-smart.js:124
var _entry$loader$match = entry.loader.match(loaderNameRe),
Some how your setup is generate a webpack1 configuration while you running on webpack2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same error when running the build
script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I dont get those errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I had a typo in '--flavor' and create-cycle-app downloaded the core flavor. You should add an error if there is an unknown option given
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Thanks for that I'll open an issue!
const spawn = require('cross-spawn') | ||
const chalk = require('chalk') | ||
|
||
const mocha = path.resolve(process.cwd(), 'node_modules', '.bin', 'mocha') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could easily break, as you are referencing an executable that we are not sure is going to be present (if you want to use mocha, it should be added as dependency I guess)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, forgot to add it
There still one thing that I'm thinking, don't you think that the webpack.config should still be hidden behind the flavor module until eject happen? (same for all the devDependencies added to the package.json as those are dependencies related to the flavor (used by various scripts) not the the app). The idea is that until you eject, you can simply update the flavor to get all the benefit of having someone mantaining it, and you only care about the dependencies you are using for building your app. |
It is currently. Normally the config lives only in the modules and the scripts just use it. Eject copies it over. |
Given the amount of to and fro on this I'd love to see a guide for making a flavor once this one is complete. Just bullets would probably do with refs to this one. |
@SteveALee good idea, do you want to open an issue for that? |
I have to update the tslint.json and then everything should be fine |
Awesomeness @jvanbruegge seems pretty solid to me. Feel free to merge it once ready. |
OK, merging now |
@SteveALee i've ported over from the create-cycle-app repo the documentation relative to creating custom flavors, and added a list of the ones we have. Feel free to edit and improve it |
That's great! Also the table is very useful. Actually I think @jvanbruegge's will work fine for me. I'll raise an issue. What is the way to menton a specific lerna package in a monrepo's issues? Just stick it in the subject in []? |
^ yes @SteveALee |
This is not finished yet, have to add package.json dependencies and I have to adjust eject.js