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

JSPM - package.json main point to dist #4460

Closed
stephenlautier opened this Issue Oct 12, 2015 · 28 comments

Comments

Projects
None yet
5 participants
@stephenlautier
Copy link
Contributor

stephenlautier commented Oct 12, 2015

Hi

I've installed this library using JSPM, and currently the package.json main: is set to "./lib/index.js"

Can it be changed as the bower.json instead to point correctly to the dist e.g. "main": "dist/pouchdb.js",?

As otherwise it fails to loads the dependencies, unless I'm doing something wrong; However by simply changing to the dist its working correctly.

Thanks!

@daleharvey

This comment has been minimized.

Copy link
Member

daleharvey commented Oct 12, 2015

I am fairly certain that is the wrong thing to do, pouchdb works an npm package and https://github.com/pouchdb/pouchdb/blob/master/lib/index.js is our entry point, It souldnt like jspm needs some configuration or fixing to load dependencies properly

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Oct 12, 2015

Yup, exactly, it would break for npm/node users. We have Bower support, which uses a bower.json file that has its main pointing to "dist/pouchdb.js". Maybe there is a similar thing for JSPM?

@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 12, 2015

hmm ok, I will try to figure out how its supposed to be for JSPM. If I have an update I will let you know. Thanks

@bdchauvette

This comment has been minimized.

Copy link
Contributor

bdchauvette commented Oct 14, 2015

From the JSPM docs, it looks like you should be able to use a JSPM-specific override for anything, including main:

{
  "main": "./lib/index.js",
  "jspm": {
    "main": "dist/pouchdb.js"
  }
}
@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 14, 2015

I was checking their docs and also saw that, which would be exactly as I was proposing without skrewing NPM 👍 .
What I'm not sure is, what would be the best approach, simply specify the dist or list dependencies? Does the dist/pouchdb.js contains dependencies within in?

@daleharvey

This comment has been minimized.

Copy link
Member

daleharvey commented Oct 15, 2015

dist/pouchdb.js is totally self contained distribution that includes all the dependencies, happy to add that to our package.json if it helps

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Oct 15, 2015

If someone who uses jspm would open a pull request and test it, that would be lovely. :) LGTM

@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 15, 2015

When I go home I can try this :)

@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 15, 2015

It works good by doing as @bdchauvette suggested.

I'm a bit lost to request the PR though, as after doing the changes and managed to release it, a lot of changes were submitted automatically for the prerelease not sure how to exclude them.
Not sure if I did something wrong

Below there's a link to my fork compare changes
master...stephenlautier:master

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Oct 16, 2015

Hi @stephenlautier, it looks like a lot of our generated files got added? If all else fails, you can just edit the package.json directly from the Github UI and we'll be happy to accept that as a PR: https://github.com/pouchdb/pouchdb/edit/master/README.md

@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 16, 2015

Ok done it @ #4476. Cheers

daleharvey added a commit that referenced this issue Oct 16, 2015

@daleharvey

This comment has been minimized.

Copy link
Member

daleharvey commented Oct 16, 2015

Fixed by 9263a3c

@daleharvey daleharvey closed this Oct 16, 2015

@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 18, 2015

Do you have a plan when you will release again? This won't work unless its released, just wanted to point out. Thanks

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Oct 19, 2015

We release once per month, on the first Monday of the month.

@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 19, 2015

Great, thanks once again.

@Hypercubed

This comment has been minimized.

Copy link
Contributor

Hypercubed commented Oct 20, 2015

@stephenlautier How are you installing pouchdb?

jspm install pouchdb=npm:pouchdb should install all dependecies. If however, you want jspm (for browser compatability reasons) to use the dist version you can add the jspm.main (as you have done) but should also override the dependecies to avoid installing all dependencies that are already included in dist.

@Hypercubed

This comment has been minimized.

Copy link
Contributor

Hypercubed commented Oct 20, 2015

I just threw this together: https://gist.github.com/Hypercubed/d3671f30905adafd3c9c

I installed pouchdb using jspm install pouchdb=npm:pouchdb, since the change above is not yet published it installed pouchdb@5.0.0 (along with all dependencies, you can see that in the config.js) and pointed SystemJS to the CommonJS version (lib/index.js).

I'm not familiar with pouchdb so I haven't tested any features, just copied the example. Unless there is a specific reasion to point jspm to the dist version you should stick with commonjs.

@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 20, 2015

@Hypercubed I've used that also jspm install pouchdb=npm:pouchdb, which downloads the dependencies, however they weren't getting mapped in JSPM I believe.

Meaning if you have the config+jspm_packages not within the root it wasn't working (at least for me). I'm using asp.net5 and my jspm_packages + config are not within the root ( / ), they are within /wwwroot/.
So at runtime, all NPM dependencies were not being fetched correctly by JSPM, they were being fetched e.g. /lie.js instead of /jspm_packages/npm/lie/lie.js (this was just an example) but that was my issue.

If you can try something similar that would be great.

@Hypercubed

This comment has been minimized.

Copy link
Contributor

Hypercubed commented Oct 20, 2015

You must have the same issue with all jspm installs not just pouchdb. Sounds like you have a misconfgured baseURL.

@Hypercubed

This comment has been minimized.

Copy link
Contributor

Hypercubed commented Oct 20, 2015

Remember the JSPM cli downloads modules (and dependencies) and generates a config.js file. This config.js file is used by SystemJS to map module names to files. Check your baseURL in the jspm section of your package.json and in the generated config.js. Make sure you are loading the config.js. Do you have a repo we can look at?

Edit: also look at the config.js file, are dependencies listed there?

@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 20, 2015

Yes, I have quite good knowledge of SystemJS (well recently), not a super pro but I know quite a bit in config.js not a total newb :). My project contains quite a bit of dependencies (around 20+) all of them works correctly including Aurelia, bluebird, moment, etc...

Unfortunately this specific project is private :(. I will give it another try maybe it works, I keep you updated. Thanks for the help 👍

@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 20, 2015

Ok @Hypercubed, its actually working now as you said using jspm install pouchdb=npm:pouchdb. Not sure why it wasn't working before but I'm pretty sure it wasn't working as I've tried couple several times :/.

To complicate a bit more things I'm running this using TypeScript + was running within karma/jasmine tests.

@stephenlautier

This comment has been minimized.

Copy link
Contributor

stephenlautier commented Oct 20, 2015

Awesome I just remembered what I was doing and tried it again.
I was installing using
jspm install pouchdb=github:pouchdb/pouchdb
instead of
jspm install pouchdb=npm:pouchdb.

That way seems to download less way dependencies and doesn't add them to the config.js map section. Not sure if its a bug from JSPM or needs configuration set specifically for JSPM, but I believe it should fallback and use npm packages automagically.

Thanks @Hypercubed 👍 mystery solved.

@Hypercubed

This comment has been minimized.

Copy link
Contributor

Hypercubed commented Oct 21, 2015

I think the only reason to use the github registry is to install bower packages not on npm. I belive @guybedford said as much somewhere... I can't find it now so don't quote me quoting him.

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Oct 21, 2015

Apparently somebody also registered PouchDB with Github? http://kasperlewau.github.io/registry/#/?q=pouchdb

@Hypercubed

This comment has been minimized.

Copy link
Contributor

Hypercubed commented Oct 21, 2015

Yeah, maybe that should be removed. If someone does jspm install pouchdb (not specifing github or npm) they get a warning:

warn github:pouchdb/pouchdb@5.0.0 dependency installs skipped as it's a GitHub package with no registry property set.
     If the dependencies aren't needed ignore this message. Alternatively set a registry or dependencies override or use the npm registry version at jspm install npm:pouchdb@^5.0.0 instead.
@Hypercubed

This comment has been minimized.

Copy link
Contributor

Hypercubed commented Oct 21, 2015

From https://github.com/jspm/registry:

The registry provides a simple mapping in the file registry.json, which maps a shortname (bootstrap) to a canonical endpoint name (github:twbs/bootstrap). It is simply a convenience for installing through jspm.

This line (https://github.com/jspm/registry/blob/master/registry.json#L347) should probabbly be deleted or changed to "pouchdb": "npm:pouchdb".

And this

"main": "dist/pouchdb.js"
should removed.

nolanlawson added a commit to nolanlawson/registry that referenced this issue Oct 22, 2015

Use npm version of PouchDB
See discussion in pouchdb/pouchdb#4460 (comment). It's much less problematic for PouchDB users to use the npm version rather than the Github version.
@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Oct 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment