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

Key will be null for commonjs, not value #12

Merged
merged 3 commits into from
Apr 3, 2015
Merged

Key will be null for commonjs, not value #12

merged 3 commits into from
Apr 3, 2015

Conversation

JesseFarebro
Copy link
Contributor

A little overweight I thought for CommonJS modules value would be the module name ex.

Key = Handlebars
Value = null

When in reality, key is the module value. There was no test case for CommonJS modules and streams so this went uncaught by me. Sorry about this. Here is a new PR to fully fix #10

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 09b4277 on JesseFarebro:key-null into d902298 on wbyoung:master.

@wbyoung
Copy link
Owner

wbyoung commented Mar 30, 2015

@JesseFarebro a test case for CommonJS would be welcome as well. Feel free to add one if you have time.

@JesseFarebro
Copy link
Contributor Author

@wbyoung I will add a commit on to that branch tonight for you with a test case!

@wbyoung
Copy link
Owner

wbyoung commented Mar 31, 2015

@JesseFarebro thanks!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ddca9a9 on JesseFarebro:key-null into d902298 on wbyoung:master.

@JesseFarebro
Copy link
Contributor Author

@wbyoung Test case is done, following the same structure as your previous stream test case.

As well through writing the test case if you have multiple CommonJS requires they were delimited by a comma due to this:

return requires + ' ... ';

If you add a string to an array it apparently gets delimited by commas. Learn something new everyday. Anyways I joined them by nothing and it was looking a lot better!

If you need any changes let me know!

@wbyoung wbyoung merged commit ddca9a9 into wbyoung:master Apr 3, 2015
@wbyoung
Copy link
Owner

wbyoung commented Apr 3, 2015

Thanks so much, @JesseFarebro. Changes have been published in 0.1.3.

@JesseFarebro JesseFarebro deleted the key-null branch April 6, 2015 19:20
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.

Allow clearing of require values set by plugins
3 participants