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

Typescript #102

Merged
merged 5 commits into from
Apr 10, 2017
Merged

Typescript #102

merged 5 commits into from
Apr 10, 2017

Conversation

jvanbruegge
Copy link
Collaborator

@jvanbruegge jvanbruegge commented Apr 10, 2017

I've added the typescript templates, but wasnt able to test them see #101 (comment)
fixes #92

@nickbalestra
Copy link
Collaborator

Ah, now I see why :) Have you tried this:

  • with prompts ahead: node ~/path/to/packages/create-react-app appName --flavor ~/path/to/packages/cycle-scripts --forceprompt

  • without prompts: node ~/path/to/packages/create-react-app appName --flavor ~/path/to/packages/cycle-scripts

@jvanbruegge
Copy link
Collaborator Author

jvanbruegge commented Apr 10, 2017

Oh, you are right, the new version is not released yet. I'm dumb

@jvanbruegge
Copy link
Collaborator Author

Ok, this works now

Copy link
Collaborator

@nickbalestra nickbalestra left a comment

Choose a reason for hiding this comment

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

Perhaps you want to also add webpack configurations for typescript within this PR?

}
}

const replacements = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't make sense to just move replacements within template as the dependencies cofnig is not related to this?

@@ -2,10 +2,6 @@ const dependencies = {
basics: [
'@cycle/[email protected]'
],
language: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure there won't be any specific language dependency that we'll need to install? (I didn't know about typescript to much that's why i left this here)

Copy link
Collaborator Author

@jvanbruegge jvanbruegge Apr 10, 2017

Choose a reason for hiding this comment

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

yes, but as this file is splitted you can add them to the basics array. We could split the file, so the basics, and stream lib stuff is common and there is an additional entry in the flavor.js for language specific imports

@nickbalestra
Copy link
Collaborator

I'll also avoid to use .jsx/.tsx extension (although supporting it in the webpack config)

@jvanbruegge
Copy link
Collaborator Author

typescript will throw an error if you use tsx syntax in a ts file

@jvanbruegge
Copy link
Collaborator Author

and i would rework the webpack configs. We should only maintain 1, not 4

@nickbalestra
Copy link
Collaborator

nickbalestra commented Apr 10, 2017

All good, I left you some minor comments.
mainly:

  • I wouldn't have a template/config/<language> structure to put inside of it a global configuration to handle both dependencies and template strings (replacements), as it doesn't make much sense as we already have template/src/<language> in there make sense to have only replacements (template strings) and live dependencies config where they were, or somewhere else, if you find a better place.

  • if tsx is required should we forcely do the same for jsx? perhpash consistency is important perhpash not as we tailoring two different ecosystems, so better to align to best-practices accordingly? dunno i don't have a clear answer

  • I don't agree in having 1 webpack.config. I think we should be able to tailor and maintain them seperately as we could fine tune them accordingly and the user of one or the other language should get only relative things to him...it also make it very easy to eject so. Therefore I don't know if the effort in having 1 config make sense...we could also get there later. I'll try not to over engineer upfront

@jvanbruegge
Copy link
Collaborator Author

jvanbruegge commented Apr 10, 2017

  1. Yes, we can add the dependencies in a common place. We can leave a language specific dependency field in there
  2. Yes, I would clearly seperate jsx and js
  3. Im even not sure if we should maintain one. Webpack config shouldn't be done by everyone again and again. webpack-blocks removes boilerplate while maintaining the possibility to add custom config just as easy as before. We would basicly outsource this part to a different open source project, so we can benefit from the blocks other people did without copy and paste. Even people like Dan Abromov suggest using the project. Plus is makes different configs easier, as you only have to create an array containing the blocks. I also disagree that we have to fine tune anything. The only difference between languages is the loader. Typescript users want to be able to just bundle typescript and javascript files (libraries). Js users dont need the typescript loader. Thats the only difference you have

@nickbalestra
Copy link
Collaborator

Let's move 3 into a separate pr and discuss it there? There are enough of both pro and cons to be discussed separately without having to block this PR. I would love to first have everything in and then see where make sense to add improvements or not, lets wait till we have the whole picture with tests ecc to decide about this.

@jvanbruegge
Copy link
Collaborator Author

ok, but then i would not add the webpack config just now to prevent unneccesary work now. This wont be released anyway as long the webpack part is not decided

@jvanbruegge
Copy link
Collaborator Author

for 3: #103
last commit addresses 1
I would leave 2 as it is

@@ -0,0 +1,24 @@
const dependencies = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this into the /configs/flavor.js perhaps now we could simply rename it into dependencies.js ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also can we simply use template/src/<template> instead of having a whole seperate structure just for those files? as they are strictly related to the templates. Perhaps we could renane this file into templateStrings.js ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. thought it is cleaner to seperate it, dont mind really
  2. My reason is that it is not part of the template source and should not be copied over, we could however remove the structure, move the files one directory up and rename them to <language>.js

Copy link
Collaborator

Choose a reason for hiding this comment

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

My rationaled on why I think they are part of template sources: Ones are precompiled template functions, the other are template strings (replacement), together they form the template sources.

Copy link
Collaborator

@nickbalestra nickbalestra left a comment

Choose a reason for hiding this comment

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

Noice!

@nickbalestra
Copy link
Collaborator

Thanks a lot for this @jvanbruegge !

@nickbalestra
Copy link
Collaborator

I'll give it a couple of tests and if all ok, merge it

@nickbalestra
Copy link
Collaborator

nickbalestra commented Apr 10, 2017

I got this when I try to run it:

Error: Cannot find module '../../configs/flavor'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (~/Projects/playground/cca/cca03/node_modules/cycle-scripts/scripts/init/setup.js:8:22)
    at Module._compile (module.js:571:32)

@jvanbruegge jvanbruegge force-pushed the typescript branch 2 times, most recently from 9be47f8 to 278cdc2 Compare April 10, 2017 11:59
@jvanbruegge
Copy link
Collaborator Author

forgot to edit the path after rename

@nickbalestra nickbalestra deleted the typescript branch April 10, 2017 12:12
perjerz pushed a commit to perjerz/create-cycle-app that referenced this pull request Nov 12, 2018
…-patch-1

Add dat-installer, cycle-hn and cycle-storageify to the list
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.

[CCA-Diversity] Bring typescript src into core-flavor
2 participants