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

Switch order of module pattern checks for more consistent UMD. #92

Merged
merged 2 commits into from May 20, 2016
Merged

Switch order of module pattern checks for more consistent UMD. #92

merged 2 commits into from May 20, 2016

Conversation

kaidjohnson
Copy link

I was having errors thrown bundling loglevel in a library that was then being bundled in an application package and found that reversing these checks solves the issue for us.

Based on this conversation: umdjs/umd#22 it seems that checking for AMD before CommonJS is a safer paradigm to follow.

@pimterry
Copy link
Owner

That sounds good, but this change has broken all the tests. Can you take a look at why? I'm happy to merge a change like this, just as long as the tests pass 😄

@kaidjohnson
Copy link
Author

Hey - that's because I'm really good at breaking tests inadvertently. Apparently, my copy-paste skills are rusty and my amd definition did not properly parallel what already existed in code.

I've updated and local tests went from red to green, so with a little bit of luck, the travis ci build should do the same.

@pimterry
Copy link
Owner

Looks great, thanks! Merged.

@pimterry pimterry merged commit 2c6190f into pimterry:master May 20, 2016
@JoHense
Copy link

JoHense commented May 23, 2016

Thanks for this fix!
@pimterry do you plan to do a minor release in the near feature? I would like to use loglevel with AMD as soon as possible :-)

@pimterry
Copy link
Owner

All good now; this is now up and released as 1.4.1. Let me know if you have any other trouble.

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

4 participants