Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Add support for CommonJS, and publish to NPM #1329

Merged

Conversation

interactivellama
Copy link
Contributor

We may promote use of the more feature rich npm module workflow in the future instead of bower. This at least opens the door to that possible effort. This does not however make the library compatible with CommonJS, yet though.

@vernak2539
Copy link
Contributor

I feel this should be done all at once. If you do this, you can just publish it, that's it. It won't really work, since nothing has been done to enable it for CJS.

I spent a few hours going over what needs to be done to get it working.

  1. bootstrap and jquery need to be added as dependencies (should also remove server-static and connect since they are in the wrong spot if this is going on npm).
    • jquery@1.9.1 does not support CJS
  2. in every js file, a if else needs to be added to check for module and module.exports, then actually use the factory with "node style" requiring of jquery
  3. a main file needs to be added to the package.json
    • should probably also create a dist npm file that requires everything instead of trying to use the ./js/all.js for both browser, AMD, and CJS
  4. probably a few more things....

Was going to try and work on this sooner than later, but above is the general gist to getting it working

@interactivellama
Copy link
Contributor Author

Thanks Alex for delimiting these. I agree it might piss some people off to publish but not support commonJS. I'll modify this PR with the above suggestions.

@scott-joe scott-joe added the Hold label Jun 2, 2015
@swilliamset swilliamset removed this from the 3.8.0 milestone Jun 3, 2015
@interactivellama
Copy link
Contributor Author

@vernak2539 It appears that even though Bootstrap "supports" CommonJS packaging, it still is asking for jQuery on the window object. In Fuel UX, AMD users can get around this with requirejs shim in their config object.

Do we need to to wrap BS somehow ourselves like we suggest with AMD or is it OK to depend on the global for jQuery? I am kind of new to CommonJS on the front-end space, so I wanted to see if you had any common usage suggestions.

@interactivellama
Copy link
Contributor Author

@vernak2539 I have a WIP branch here for you to test. I didn't want to force push it here, because it's got a lotta cruft in it that's not going in the library in order to just use it (and I didn't want to scare anyone into thinking it was all going in). It has Fuel UX controls running as commonJS modules in /index-cjs.html.

I don't think it would be good idea to duplicate all the tests just in a commonJS format. In this example, Bootstrap is using jQuery on the window, but Fuel UX should not be.

@vernak2539
Copy link
Contributor

@interactivellama yeah ill take a look today. Definitely should be no need to duplicate the tests.

@interactivellama
Copy link
Contributor Author

One thought I had is that I didn't add the CommonJS wrapper to dist/js/fuelux.js would anyone expect that to be present? I would think they would always use the the /js via node_modules, since "small is good" and "combined is bad" for CommonJS.

@vernak2539
Copy link
Contributor

I think that looks fine. You'll probably have to have a gist or something demonstrating how it needs to be setup with jquery, bootstrap, etc.

not sure what you should do about including bootstrap. usually in these kind of things, it would be stated as a dependency, and thus installed on the npm install. But, this is a repo for all the various types. May want to look into that, and making sure only real dependencies are listed in the package.json

Edit: bootstrap and jquery would have to be added as deps probably

@vernak2539
Copy link
Contributor

One thought I had is that I didn't add the CommonJS wrapper to dist/js/fuelux.js would anyone expect that to be present? I would think they would always use the the /js via node_modules, since "small is good" and "combined is bad" for CommonJS.

Since you've set the "main" script in the package.json, all that's required is require('fuelux') and it should sort the correct package. If people use a relative path when trying to access it, they're using it wrong

Edit: and you should never have to bundle it for them. just leave that npm.js file there, and then when they include via browserify, it should do all the work for them in their implementation

@interactivellama
Copy link
Contributor Author

Looks like I forget to push the latest updates to package.json that add jquery and bootstrap as dependencies.

@vernak2539
Copy link
Contributor

Looks like I forget to push the latest updates to package.json that add jquery and bootstrap as dependencies.

yep that looks good

@interactivellama interactivellama force-pushed the remove-private-package branch from 48564bd to 9db7837 Compare June 15, 2015 17:00
@interactivellama
Copy link
Contributor Author

Updated with actual code to support commonJS + basic unit tests ran within the validate-dist grunt task. This also fixes #950.

Welcome to 2012 Fuel UX! 😺

@interactivellama interactivellama changed the title Make the library publishable to NPM registries Add support for CommonJS, and publish to NPM Jun 17, 2015
@interactivellama interactivellama removed this from the 3.9.0 milestone Jun 25, 2015
@interactivellama interactivellama modified the milestones: 3.10.0, 3.9.0 Jun 25, 2015
@interactivellama interactivellama force-pushed the remove-private-package branch 2 times, most recently from 2e8023b to 547fbd1 Compare June 30, 2015 20:46
This creates a test like the Browser globals test that detects if the CommonJS bundle created with browserify has initialized the plugins. It gitignores the bundled file that is created within the validate-dist task.
@interactivellama interactivellama force-pushed the remove-private-package branch from 547fbd1 to f02ddbd Compare July 6, 2015 17:39
@interactivellama
Copy link
Contributor Author

@swilliamset Updated with master branch. Also tests finally passed by getting grunt-browserifyon the heroku app.

swilliamset pushed a commit that referenced this pull request Jul 9, 2015
Add support for CommonJS, and publish to NPM
@swilliamset swilliamset merged commit 90542e1 into ExactTarget:master Jul 9, 2015
@interactivellama interactivellama deleted the remove-private-package branch April 4, 2016 15:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants