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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added source-map option #79

Merged
merged 5 commits into from
Feb 21, 2018
Merged

Conversation

cristianbote
Copy link
Contributor

I was missing this option, and I noticed @lukeed created a issue for it here #73 and you know, I had a few free minutes and why not, lend a hand. 馃槃

Let me know if this is out of scope, or not really something you'd like to go towards.

Copy link
Contributor

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Cool~! 馃帀

src/index.js Outdated
@@ -310,7 +310,7 @@ function createConfig(options, entry, format, writeMeta) {
strict: options.strict===true,
legacy: true,
freeze: false,
sourcemap: true,
sourcemap: options.sourcemap===true || options.sourcemap===undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

This can just be options.sourcemap since the default is set to true 馃槃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that was my assumption as well, at first 馃檪. Seems that the cli would have it defined from sade as true, but running it through the node package, would end up undefined.

Did I get it right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's because your first attempt was using sourceMap -- I just checked out your current branch it's working as expected. There's no need for any === checks 馃憤

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, seems like @developit did some trickery check there to cover both cases, for default true options 馃槃just checking it that it's not false options.sourcemap!==false. That covers both cases, cli and node.

Node it's different, the options object it's not validated by sade just a plain config option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right about here https://github.com/developit/microbundle/blob/master/src/index.js#L265 I was looking how the other always true values are behaving. And here's how the options are passed to microbundle function https://github.com/developit/microbundle/blob/master/test/index.test.js#L29.

Copy link
Contributor

Choose a reason for hiding this comment

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

Those existed for Yargs, aka before the switch to Sade 馃槃 I didn't bother converting them to make the PR diff easier to follow.

Sade uses mri under the hood, which will always ensure that a flag value will be boolean|string|number if specified or if an initial value is provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, nice! That makes sense. Also, kudos on the mri 馃槂that's impressive perf, you got there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, thanks 馃檭 Flag parsing perf doesn't matter too much for the majority of cases, but is nice to have. The simple utilities of type-casting & assumed booleans are more useful... allowing you avoid all these !== manual checks in your apps.

@cristianbote
Copy link
Contributor Author

Hey @lukeed if there's anything else, let me know ;)

@developit developit merged commit 87d7051 into developit:master Feb 21, 2018
@developit
Copy link
Owner

Thanks! Totally needed this today haha

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.

None yet

3 participants