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

jspm section in package.json #2467

Closed
jbanety opened this issue May 15, 2019 · 22 comments
Closed

jspm section in package.json #2467

jbanety opened this issue May 15, 2019 · 22 comments
Labels

Comments

@jbanety
Copy link
Contributor

jbanety commented May 15, 2019

Hi @guybedford,
I created several jspm packages that are used as dependencies for my main application.
With jspm 0.17 there was a jspm section in the package.json file.
With 2.0, when I install the jspm packages, the dependencies in the jspm section are not installed with.
Do I have to put these dependencies at the root of the package.json file? But I do not want them to be installed with yarn install because this packages are only for the front.

Thanks.

@guybedford
Copy link
Member

jspm 2.0 still supports the "jspm" section in the package.json, where dependencies will be installed from and added to there.

Usually I do this via echo '{ "jspm": { "dependencies": {} } }' > package.json and then jspm install will always use that.

If there's a way you're doing this that isn't working or is behaving unexpectedly perhaps it's a bug or usability issue I haven't seen.

@jbanety
Copy link
Contributor Author

jbanety commented May 15, 2019

When I install a package with dependencies in jspm section, these dependencies are not installed.

@guybedford
Copy link
Member

Ahh, right, the jspm field is not being respected in dependencies. Yes, that is a bug!

@jbanety
Copy link
Contributor Author

jbanety commented May 15, 2019

Can you explain where I have to investigate. If I can, I'll create a PR.

@guybedford
Copy link
Member

The raw to structured package.json processing happens here - https://github.com/jspm/jspm-cli/blob/master/src/install/package.ts#L422.

It could be worth seeing if a preprocessing phase could copy down the jspm properties (noting that any of devDependencies, peerDependencies, etc should override all of them to be empty, that is dependencies are overridden as a group).

There may be some edge case complexities if this function is used in other scenarios / or in override handling, but we might be lucky with these.

@jbanety
Copy link
Contributor Author

jbanety commented May 19, 2019

Hi @guybedford,
I think I fixed this issue in the @jspm/jspm-resolve package where the jspm section was not processed in the processPjsonConfig method.

@jbanety
Copy link
Contributor Author

jbanety commented May 19, 2019

An other issue is the overrides in dependencies. They aren't considered. I have to add overrides section in the package.json of the final module.

@guybedford
Copy link
Member

Do you mean the package.json "overrides"?

There's a central overrides file in jspm at https://github.com/jspm/jspm-cli/blob/master/src/overrides.ts where we can include common overrides that would be correct and useful to all users.

The other option is to directly support overrides being added on install from dependencies, and the machinery is mostly in place to support this quite easily.

I was always hesitant to implement this though for fear of conflicts opting instead for the central registry, but we could still reconsider this.

Overrides in dependencies applied on install, would effectively be propagating into the base-level package.json "overrides". It's like peer dependencies, where conflicts just need to be resolved when this happens, but other than that it's a fairly straightforward integration.

The risk as always is that installing a dependency can potentially break your other dependencies, and this is still the thing we need to weigh up against the benefits.

@jbanety
Copy link
Contributor Author

jbanety commented May 20, 2019

Yes, it could be dangerous to apply overrides from dependencies.
Maybe we could print the packages with overrides to the user ?

@guybedford
Copy link
Member

Would your overrides be suitable for inclusion in https://github.com/jspm/jspm-cli/blob/master/src/overrides.ts? This now replaces the previous jspm/registry.

I must admit I was kind of hoping we wouldn't need overrides all that often, and that they would just be a stop-gap more than anything.

@jbanety
Copy link
Contributor Author

jbanety commented May 20, 2019

My overrides are just to use the correct ES module file in a package with overriding the main property.

@guybedford
Copy link
Member

Do you mind sharing which package? What is the consequence of not using the override?

@jbanety
Copy link
Contributor Author

jbanety commented May 20, 2019

My app is based on micro-service architecture.
I have built a scoped package named ui to share ui components which depends on vanilla-notify.
The main property of vanilla-notify is set to a non-ES compatible file, so I override the mainprop in the package.json.
Then, I have a frontend package wich depends on ui.
So when I run jspm install in frontend, I first wanted to have the ui overrides to pull up in the frontend but you're right in other case it can break other dependencies.

@guybedford
Copy link
Member

So jspm should convert the CommonJS vanilla-notify into an ES module on install, making it effectively ES compatible. Is there a reason you can't rely on that process?

@jbanety
Copy link
Contributor Author

jbanety commented May 20, 2019

I have this error :

err (jspm) Error: 'VanillaNotify' is not exported by jspm_packages/npm/[email protected]/vanilla-notify.js

with my code import {VanillaNotify} from "vanilla-notify";.
This is because I'm using the VanillaNotify class exported by src/vanilla-notify.js.
So you're right it is not a ES compatibility problem.

@jbanety
Copy link
Contributor Author

jbanety commented May 20, 2019

But I always prefer to use vendor ES module file rather than the converted version.

@guybedford
Copy link
Member

Does #2476 fix this because vanilla-notify is your own package, where you've added a "jspm" property just for jspm?

@jbanety
Copy link
Contributor Author

jbanety commented May 20, 2019

Nop. This is for the main issue : #2467 (comment)

@guybedford
Copy link
Member

Ahh right thanks.

So with these problems it's worth always putting in your mind first how we work this out under node --experimental-modules. Because the goal is to have one ecosystem not to split it.

And these problems are exactly what node --experimental-modules is grappling with right now (https://twitter.com/robpalmer2/status/1130367435020935170, https://twitter.com/MylesBorins/status/1128846502468751361).

There was a proposal to support named exports for CommonJS through a package.json declaration, which would be nice as it would be supported in overrides, but then could also be PR'd to the main repo and supported natively and in Node.js.

This is namedExports in jspm and I would still like to push for standardization on that, but we will see.

Otherwise, overriding the main can cause issues because say another dependency uses that version then they won't match up properly as discussed in Myles' thread.

I know ES modules have been pushed on their treeshaking benefits - is that the main reason you'd prefer to use the ES module version?

@irustm
Copy link
Contributor

irustm commented May 20, 2019

@jbanety
I think you just need to use transpiler

@jbanety
Copy link
Contributor Author

jbanety commented May 20, 2019

No need to use a transpiler since i'm using native es6 in latest browser.

@jbanety
Copy link
Contributor Author

jbanety commented May 20, 2019

And I'm using https://github.com/guybedford/es-module-shims for newer modules specifications.

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 a pull request may close this issue.

3 participants