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

jsfx.js can be used as both an IIFE and a node module #15

Merged
merged 2 commits into from Jan 16, 2017
Merged

jsfx.js can be used as both an IIFE and a node module #15

merged 2 commits into from Jan 16, 2017

Conversation

mtmckenna
Copy link
Contributor

Hello! I'm pretty jazzed on jsfx and looking to use it in an app that uses NPM modules to handle dependencies, and so it would be convenient if jsfx could be packaged as an NPM module as well.

In this pull request, I updated jsfx.js to be able to be executed as either an IIFE (as it is currently) or as an NPM module. I based my changes on this post from NPM and this blog post. The update should not break any compatibility with previous versions of jsfx.

If these changes work for you and you're interested, it'd be extra handy if you wouldn't mind publishing jsfx to NPM.

In case it's useful, I also created a demo app that is your index.html using the Node module version of jsfx.

Please let me know if you have any questions or thoughts!

Thank you!

McKenna

@egonelbre
Copy link
Member

Tabs, tabs everywhere :D

Otherwise it looks good.

Also, is it possible to avoid assigning version numbers to npm modules? It does not make sense for this library -- or rather it's more dangerous to put one. You should be depending on a particular version rather than 1.+... Although the API is stable, the results may not be.

@mtmckenna
Copy link
Contributor Author

Sounds good! Happy to convert the spaces to tabs. One thing--it looks like the original file uses tabs rather than spaces. Would you like me to convert the whole file? The reason I didn't do that originally was to make the diff prettier. =)

As for the version number, it looks like it's required to publish on NPM. We could set the version number fairly low if that'd help (like 0.0.1)? What do you think?

@egonelbre
Copy link
Member

Would you like me to convert the whole file?

Not sure what you mean? I meant that the diff you submitted had spaces, but these should have been tabs -- nothing more. The same for the json file. :)

(PS: I really don't care about the tabs/spaces, as long as it is consistent, and for whatever reason I ended up using tabs in this project.)

As for the version number, it looks like it's required to publish on NPM. We could set the version number fairly low if that'd help (like 0.0.1)? What do you think?

1.0.0 is fine then, as it would be more aligned with versioning spec.

@mtmckenna
Copy link
Contributor Author

Ah--I misinterpreted and though you preferred spaces everywhere. =) Changed the spaces to tabs. Let me know if you'd prefer I squashed the commits into one!

@egonelbre egonelbre merged commit 35d2b2c into loov:master Jan 16, 2017
@egonelbre
Copy link
Member

I will take a look at publishing to npm tomorrow.

@mtmckenna
Copy link
Contributor Author

Awesome, thank you very much!

@egonelbre
Copy link
Member

I published it under loov-jsfx, because I wasn't able to get scoped packages working.

@mtmckenna
Copy link
Contributor Author

Thank you!!

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

2 participants