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

include jspm package configuration #11617

Merged
merged 1 commit into from
Nov 30, 2013
Merged

include jspm package configuration #11617

merged 1 commit into from
Nov 30, 2013

Conversation

guybedford
Copy link
Contributor

With this configuration in the package.json, Bootstrap can be included in a page through jspm with:

http://jsbin.com/osIYuDOJ/1/edit

This includes minification with source maps, shimming the jquery dependency, setting the main entry point and loading resources from the "dist" folder.

For example, this approach is used by http://getkickstrap.com/, a Bootstrap framework.

Bootstrap itself isn't downloaded as a clone of the Github repo, rather the Github release is detected and used instead.

Questions welcome, I know package.json littering can be a contentious issue. There are other options as well so happy to discuss this.

@cvrebert
Copy link
Collaborator

LGTM.
/cc @fat @mdo

@mdo
Copy link
Member

mdo commented Nov 27, 2013

What's the usage around jspm? Never heard of it.

@guybedford
Copy link
Contributor Author

@mdo if you're worried about the "jspm" prefix, the same configuration also works at the base level of the package.json. As much as possible it extends existing properties like "directories". The only new properties are "shim" and "buildConfig" in this case. Let me know if you want to go this route.

We're in early adoption - I completely understand if you only want to support something completely established, it just makes it much easier to maintain having this information in the package itself instead of needing the override service.

@mdo
Copy link
Member

mdo commented Nov 27, 2013

The actual code isn't really of concern to me, only that we're not getting ahead of ourselves in supporting something new and not screwing up anything for the rest of the package.json.

@cvrebert
Copy link
Collaborator

npm install gives no warnings or errors with the modified package.json.

@guybedford
Copy link
Contributor Author

package.json was designed in CommonJS to be extended, and NPM encourages it if anything (see the comment after the third quote in this article - http://blog.izs.me/post/27987129912/tj-holowaychuk-components). I personally don't believe in having multiple package metadata configuration files (this being a different concept from build files), but I know this can be personal preference.

I suppose the question is more whether Bootstrap wants to be seen to be supporting a new technology. That's why I suggested the non-jspm prefixed base-level configuration of:

package.json:

{
    "main": "js/bootstrap"
  , "directories": { "lib": "dist" }
  , "shim": {
      "js/bootstrap": {
           "imports": "jquery"
         , "exports": "$"
       }
  }
  , "buildConfig": { "uglify": true }
}

in case this is the real issue here.

In this case consider that jspm has specified the shim and buildConfig package.json properties for use, in a way that could even be shared with other package managers if they cared to use the same configuration.

@adamjgrant
Copy link

With the code you have above, it would make sense that any such technology that loads Bootstrap as a singular package could use the configuration--it wouldn't be specific to JSPM.

mdo added a commit that referenced this pull request Nov 30, 2013
include jspm package configuration
@mdo mdo merged commit 40f86af into twbs:master Nov 30, 2013
@mdo
Copy link
Member

mdo commented Nov 30, 2013

👍

@mdo mdo mentioned this pull request Nov 30, 2013
@guybedford
Copy link
Contributor Author

Amazing, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants