yeoman / yeoman

Yeoman - a set of tools for automating development workflow
http://yeoman.io
10.08k stars 734 forks source link

yeoman init with Require.js fails #251

Closed addyosmani closed 12 years ago

addyosmani commented 12 years ago

I discovered this while implementing #246.

Basically, if you accept all yeoman init options and run yeoman server, checking the console - you'll notice that it says there is a missing config file for Require.js. Run yeoman build however and everything builds fine, so its a paths issue.

Also interesting: in your index.html file if you set the data-main for Require.js to scripts/main (or whatever, as long as its the full path) you can get server working, but then build fails.

addyosmani commented 12 years ago

More notes from our discussion about this earlier:

oliverturner commented 12 years ago

@addyosmani This has been the case since #206

See https://github.com/yeoman/yeoman/issues/206#issuecomment-7852643

addyosmani commented 12 years ago

@codedsignal ya. wasn't able to reproduce a few days ago. We need to figure this out this week.

oliverturner commented 12 years ago

@addyosmani I have no doubt that you will :)

Undying respect to you and the crew for your awesome efforts so far.

addyosmani commented 12 years ago

@mklabs I've been taking a look at this for the past few hours and I feel we're close to getting it right, or at least understanding whats been breaking.

Opting for a more explicit path in the index.html data-main (e.g scripts/main) and then a complete path (odd, but yes) in the Gruntfile, something like:

    rjs: {
      optimize: 'none',
      name: 'main',
      baseUrl: '/Users/addy/github/test/app/scripts/',
      wrap: true
    },

gives us a yeoman server that executes your modules without issues and yeoman builds correctly.

That's great, but then you try running in dist what build generated and it looks like although the usemin blocks have been processed and created the (revved)amd-app.js script file correctly, it tries to load up scripts/app.js and fails.

This was a little strange to me because I would have thought that these files would all have been combined and minified correctly. I then experimented with toggling the r.js optimize config and when you don't set this to true you do get

0786eab6.app.js     
6c094302.amd-app.js
650f0fa1.main.js    
c045fe6e.plugins.js

type files generated (which is fine).

Unfortunately, this means that we're missing something that links all of these files up together to the application. Ideally, my expectation would be they would all just be compiled and executed as needed on page load.

I remember adding an r.js specific build profile around I/O for us to workaround the usemin/rev issues and wonder if this is that problem showing its head again. Unfortunately, even that option is a bit of a challenge at the moment because of the usemin handle issue I reported a few days ago.

I'll experiment further and see if theres another workaround that can get the finally built output to do what we expect.

mklabs commented 12 years ago

Strange indeed.

I would have thought that these files would all have been combined and minified correctly.

I would have said the same thing. rjs should output in the final main.js file, all the files combined (minified should probably be left to the min task). It it's not, then we have a problem here.

Having files around that are revved shouldn't be an issue, as they shouldn't be referenced via any <script /> tag but handled as amd modules.

If you can push your test case in some branch (prob in your yeoman fork), I would be happy to try reproduce the error and investigate further with you.

addyosmani commented 12 years ago

@mklabs pushed what I have to the requirejsconfig branch of this repo.

mklabs commented 12 years ago

Thanks Addy, I'll take a look asap.

addyosmani commented 12 years ago

@mklabs were you able to find out if there was anything else behind this issue?

mklabs commented 12 years ago

Unfortunately, nope, I didn't have the chance / time to take a look (just yet).

jsoverson commented 12 years ago

@addyosmani, it looks like usemin is specifying the data-main attribute as the source to be appended instead of the built file, which is why you aren't getting the modules in the built source. The generated index defines the outfile as amd-app.js, not 'scripts/main' (the data-main attribute).

If I change line 230 in usemin.js from asset += ',' + main[1] + '.js'; to asset += ',' + output; I get a successful build (along with changing the rjs config to include name: 'main').

addyosmani commented 12 years ago

@jsoverson Thanks for looking into this!. Does that also result in a successful yeoman server temporary build too? If so, happy to make the changes needed this evening to get that patched.

jsoverson commented 12 years ago

Yes, dist/ accessed via yeoman server works the way it is expected.

addyosmani commented 12 years ago

@jsoverson if you get a chance could you verify the latest changes landed above work as expected? Everything seems to be fine other than an exception that runs regardless of build type (<WARN> Cannot read property 'stdout' of undefined Use --force to continue. </WARN>).

@sindresorhus are you able to reproduce?

jsoverson commented 12 years ago

I still need to manually add name : "main" to the rjs config for it to build. That could be a hardcoded fix in the Gruntfile.js template or it could be automatically generated in that section of usemin, but that's where I don't know best practice for Yeoman.

Personally, as an r.js user, I would prefer it to just be part of the template and avoid much internal magic that makes the config potentially more prone to later confusion or conflict.

Maybe an rjs config generator that would parse the project and offer suggestions is something that could fall into the scope of a Yeoman addon. Let me know if that's something that would make sense in Yeoman somewhere.

addyosmani commented 12 years ago

I still need to manually add name : "main" to the rjs config for it to build. That could be a hardcoded fix in the Gruntfile.js template or it could be automatically generated in that section of usemin, but that's where I don't know best practice for Yeoman.

I'll experiment with this further and land the change if its absolutely needed for the build. I think a hardcoded fix in the Gruntfile.js template would make sense here.

addyosmani commented 12 years ago

Maybe an rjs config generator that would parse the project and offer suggestions is something that could fall into the scope of a Yeoman addon.

I also like the idea of this. Lets discuss it further for the next version of the project at some point.

addyosmani commented 12 years ago

Going to leave this open for further testing. Once we're confident that yeoman init builds are working fine with the latest changes, I'll need to make them to the generators also relying on AMD/r.js for their config.

addyosmani commented 12 years ago

@jsoverson can you confirm the latest works for you?

jsoverson commented 12 years ago

Latest works for me

All Y on yeoman init yeoman server and browse /app, all amd modules load yeoman build succeeds (though I get errors on compass task and can't find glyphicons) yeoman server and browsing /dist looks good. amd-app.js is built properly and includes requirejs and all referenced modules.

addyosmani commented 12 years ago

Perfect! Thank you :)