search

duojs / duo

A next-generation package manager for the front-end
3.42k stars 118 forks source link

External Source Maps! #439

Closed dominicbarnes closed 9 years ago

dominicbarnes commented 9 years ago

This adds support for external source-maps!

The --development flag no longer implies source-maps, instead the --source-maps (or --inline-source-maps) flags must be used.

Duo#sourceMap(value) has been added to the API.

In the CLI, when printing to stdout, only inline source-maps are supported. Thus, if you only specify true, it will automatically change to inline.

BREAKING: this changes the api for Duo#run(), it now returns the same object that duo-pack@2 returns. (ie: { code, map })

dominicbarnes commented 9 years ago

I'm going to work on adding more tests for the CLI, so hold off on merging this quite yet. But feel free to discuss and suggest changes right now.

/cc @stephenmathieson @MatthewMueller @yields

dominicbarnes commented 9 years ago

Yay, the tests are failing for node 0.10 (only examples it looks like)

I'll have to look into this later tonight.

stephenmathieson commented 9 years ago

imo, --development should still imply source maps

stephenmathieson commented 9 years ago

This is awesome @dominicbarnes! I left a few comments, but overall this lgtm!

cristiandouce commented 9 years ago

IMHO atomic options are better.

As the description already says: --development just implies include development dependencies

Then, if you want development dependencies and sourcemaps you can still:

duo --development --source-maps entry.js

I believe the more verbose the options are the better.

dominicbarnes commented 9 years ago

I'm with @cristiandouce on this one, I prefer the more explicit options. (was wanting to revisit duo-pack itself to change that API to reflect this)

stephenmathieson commented 9 years ago

@dominicbarnes @cristiandouce is there ever a time when you'd want dev deps installed but not want source maps included?

cristiandouce commented 9 years ago

@stephenmathieson maybe for testing in a CI? dev deps are installed when running assert and stuff, right?

stephenmathieson commented 9 years ago

@cristiandouce would you actually actually build a file for testing in a ci differently than in your browser?

this seems silly to me:

test-browser: build/test.js
  duo test --build $< browser

test-ci: build/ci.js
  duo test --build $< phantomjs

build/test.js: test.js index.js
  duo --development --source-maps index.js > $@

build/ci.js: test.js index.js
  duo --development > $@
yields commented 9 years ago

nice!

though --development should implicitly add source-maps i think, maybe --source-map should override the inline source-map if needed.

dominicbarnes commented 9 years ago

Sounds like we have a bit of a tie going on here, would you like to weigh in @MatthewMueller? (ie: should --development automatically imply --source-map?)

dominicbarnes commented 9 years ago

And just to make sure everyone understands the exact semantics of what I've proposed here:

# generates build/index.js + build/index.js.map
duo --source-map index.js

# generates build/index.js + inline source-map
duo --inline-source-map index.js

# generates build/index.js (no source-maps)
duo index.js

I look around at a couple other libraries to see how they did this:

Looks like I adopted the same methodology as babel. (w/o even realizing it!)

yields commented 9 years ago

i agree with most of those changes, but i can't understand why the need for --inline-source-map?

seems like it would be nicer from a user point-of-view to have inlined source maps whenever --development is given, but also have --source-map to override the inlined ones...

i might be missing something

stephenmathieson commented 9 years ago

@yields i agree fully

dominicbarnes commented 9 years ago

@yields the main reason for the extra CLI flag was because having an optional value for a flag is tricky. At first I wanted to do --source-maps [inline], which meant I wanted either duo --source-maps index.js or duo --source-maps inline index.js. Unfortunately, commander would confuse index.js for the that optional bit.

So, @yields and @stephenmathieson would like something like this:

# generates build/index.js + build/index.js.map
duo --source-map index.js
duo --development --source-map index.js

# generates build/index.js + inline source-map
duo --development index.js

# generates build/index.js (no source-maps)
duo index.js

I think I still prefer the more explicit options, but I can live with either! :)

dominicbarnes commented 9 years ago

Oh btw, I've made a slight change. I've renamed the flags to be plural (since multiple entries can be processed at once)

dominicbarnes commented 9 years ago

I just had a compromise/idea, what if we support both --development and --inline-source-maps?

# generates build/index.js + build/index.js.map
duo --source-maps index.js
duo --development --source-maps index.js

# generates build/index.js + inline source-map
duo --development index.js
duo --inline-source-maps index.js

# generates build/index.js (no source-maps)
duo index.js

This also opens up the possibility for turning off source-maps. (assuming you had a reason)

# only downloads dev deps, no source-maps
duo --development --no-inline-source-maps index.js

/cc @yields @stephenmathieson

cristiandouce commented 9 years ago

@stephenmathieson what I mean is that I'd avoide sourcemaps or inline sourcemaps when building build/test.js. Yet, nevermind, It's not a big issue to me.

I'd just prefer to avoid the "implies" which rely hidden on documentation which you only check when the command fails.

If the option --development implies sourcemaps, It'd good to add the implication at it's command line description, so when duo --help I see exactly what the development flag does.

This seems silly to me:

# only downloads dev deps, no source-maps
duo --development --no-inline-source-maps index.js

having to exclude some hidden default. It's more explicit to add flags that add functionality than remove. IMHO

dominicbarnes commented 9 years ago

Hey guys, it's been almost a week and I haven't heard anything new. Since we seem to be divided, would you guys be willing to merge this as-is, and we can change it later if it turns out to be a problem?

dominicbarnes commented 9 years ago

I'd really like to hear from @MatthewMueller regarding the discussions above, but I will probably proceed with merging this sometime this weekend if I don't hear anything else.

matthewmueller commented 9 years ago

Didn't mean to leave this hanging @dominicbarnes! I'm tentatively with @yields in that --development should imply source-maps and we should make smart choices for the developer about the options of --development.

That being said, I'm not familiar with all the pros and cons of external vs internal source maps, but I think we should take a stand on how development "should" be. in other words, we should be weighing the pros and cons and making that decision so newcomers don't have to. I think what you suggested @dominicbarnes in your "compromise/idea" would work.

dominicbarnes commented 9 years ago

@duojs/owners I'm going to try and wrap this up today, here's what the final API should look like:

# generates build/index.js + build/index.js.map
duo --external-source-maps index.js
duo --development --external-source-maps index.js

# generates build/index.js + inline source-map
duo --development index.js

# generates build/index.js (no source-maps)
duo index.js

This preserves the current behavior of --development (that implies inline source maps) while allowing for external source-maps with an extra flag. Success!