search

longbill / jquery-date-range-picker

A jQuery plugin that allows user to select a date range
MIT License
1.12k stars 579 forks source link

Add a `main` field to `package.json` #441

Closed monovertex closed 5 years ago

monovertex commented 5 years ago

We have this package as a NPM dependency, but directly to the repository, because the NPM-published version seems to be outdated.

We're using this package in an application where the build tools use https://github.com/webpack/enhanced-resolve. At some point the tools try to resolve jquery-date-range-picker, but fails because there's no main, browser or module field in the package.json file (see #438).

This PR adds a main field that points to the main entry point of the package, so that it may be properly resolved when necessary.

monovertex commented 5 years ago

@holtkamp, ping?

holtkamp commented 5 years ago

thanks for the reminder,

please see

https://github.com/longbill/jquery-date-range-picker/issues/438 and https://github.com/longbill/jquery-date-range-picker/issues/373

It might be better to use https://github.com/christophior/jquery-daterangepicker

Don't you think?

monovertex commented 5 years ago

@holtkamp, thank you for the answer!

I saw #438, I referenced it in the PR description as well.

Regarding that published NPM package, I'd rather use this repository rather than a third party, especially one that doesn't seem to be actively maintained (from what I read in #373). NPM allows you to specify a repository as a package source directly, but it's currently not working for this repository simply because of that missing main property.

Here's how we're specifying this dependency:

"jquery-date-range-picker": "longbill/jquery-date-range-picker#v0.18.0",

From what I see, there's no downside to my PR and it would remove the need of a 3rd-party published package.

holtkamp commented 5 years ago

@monovertex thanks for the detailed answer, my NPM knowledge is quite limited 😄

Merged it, that's it? Does it need to be tagged as a new version as well?

And currently the main section refers to src/jquery.daterangepicker.js. Is there any particular / documented reason why this is not dist/jquery.daterangepicker.min.js ?

monovertex commented 5 years ago

@holtkamp, we would appreciate a tag 😁 Otherwise we need to reference master, which would not freeze the version.

Regarding your question, we're importing the module, so in dev builds we keep the code unminified. We minify the entire bundle ourselves during build for production, which I figure is the usual use case when using build tools and importing modules.

holtkamp commented 5 years ago

@monovertex ok, thanks. FYI trying to tag a new minor version 0.19.0. But since the version number is also inside the dist files, this requires me to rebuild using gulp. And that seems broken / not working on my environment.

Also a fresh git clone, npm install is not working 😩

It seems you are more proficient at this kind of NPM / build stuff, would you consider to be maintainer of this library? Also see this request: https://github.com/longbill/jquery-date-range-picker/issues/404

monovertex commented 5 years ago

Okay, don't worry about the new tag. We are already using the fix on a fork of our own, so we can switch to this repo when a future release comes. I'll continue the discussion regarding maintainers in #404, so we keep the messages relevant.

holtkamp commented 5 years ago

Okay, don't worry about the new tag.

A brew update installed a new version of NPM: 10.9.0, using which I was able to run npm install and gulp again, which allowed me to run npm version minor to tag a new minor version: https://github.com/longbill/jquery-date-range-picker/releases/tag/v0.19.0