Skip to content
This repository has been archived by the owner on Mar 15, 2018. It is now read-only.

use r.js proper rather than amd-optimize (bug 1092718, 1078093) #4

Merged
merged 1 commit into from
Nov 6, 2014

Conversation

ngokevin
Copy link
Contributor

@ngokevin ngokevin commented Nov 6, 2014

using rjs is so much easier and powerful. It's Gulp's recommended way to build require projects. At first I went with gulp-requirejs which was a bad idea since it was blacklisted and didn't mesh gulp+rjs well. Then I went with amd-optimize which doesn't have enough features. So rjs does everything for us.

  • Handles including modules like almond without concatting
  • Allows an insertRequire so we don't need init.js
  • Handles source maps easily
  • Removes views/tests easily while stubbing, reducing our include.js size by a tiny bit

var argv = require('yargs').argv;
var clean = require('gulp-clean');
var concat = require('gulp-concat');
var eventStream = require('event-stream');
var extend = require('node.extend');
Copy link
Contributor

Choose a reason for hiding this comment

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

it's still really weird to me that we don't list the dependencies in the package.json - only in Fireplace's package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

marketplace-gulp is a bower repository so it doesn't have a package.json. i've been looking for ways to share npm deps between projects.

Copy link
Contributor

Choose a reason for hiding this comment

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

but there's node code in the bower repo. so that complicates things. right?

I realise you wouldn't necessary want to npm install all of these dependencies below, since they're not used in this repo. but I'm unsure of what's the best practice in this case (besides not use bower like this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah...it's weird since we're distributing a common gulpfile across multiple projects. Few people have needed to do that so there's not good facilities to do so. So I don't have a clear solution for managing marketplace-gulp dependencies within projects besides maintain them manually (which after everything settles I hope we don't have to do much).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering what it would look like if we made this an npm module...since you're right it's Node code.

@cvan
Copy link
Contributor

cvan commented Nov 6, 2014

r+wc I'll need to test it locally but trust ya

baseUrl: config.JS_DEST_PATH,
findNestedDependencies: true,
generateSourceMaps: true,
include: ['lib/almond', 'main'],
Copy link
Contributor

Choose a reason for hiding this comment

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

still including lib/almond?

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, what's paths.almond?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might not need paths.almond anymore since I'm changing the Commonplace configuration to copy almond into the lib folder for source mapping reasons. I'll get rid of paths.almond

@ngokevin
Copy link
Contributor Author

ngokevin commented Nov 6, 2014

Thanks! I hadn't thought much about making this an npm module instead, because it really should be. I was focused on the fact that gulpfiles just live in the project and aren't required by other stuff. But it'll work well in npm.

ngokevin added a commit that referenced this pull request Nov 6, 2014
use r.js proper rather than amd-optimize (bug 1092718, 1078093)
@ngokevin ngokevin merged commit 52ef7b4 into master Nov 6, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants