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

Ignore dev files in published package #80

Merged
merged 1 commit into from
Feb 1, 2018
Merged

Ignore dev files in published package #80

merged 1 commit into from
Feb 1, 2018

Conversation

jariz
Copy link
Contributor

@jariz jariz commented Feb 1, 2018

These files are not used by library consumers and therefore should be ignored by the published files.
(I'm using babel from the project that consumes this library and babel attempts to use the .babelrc file, which is an option that can't be disabled - not in my setup, at least.)

Furthermore, I propose the following changes (which I'll make a separate PR for if agreed to):

  • It is fairly common practice in the npm ecosystem to make your main entry point adhere to ES5 code with require calls for external dependencies.
    I suggest pointing the main entry point to the compiled dist file.
  • The reliance upon a global variable (jQuery & $) is making this project impossible/very hard to use when bundling without explicit modifications to standard loader logic.
    I suggest adding jquery as a peerDependency and adding a import for it on the places the global var is referenced.
    Additionally, an default export could be added to src/index.js that would be a patched version of jQuery with the filterizr plugin added to it.

@giotiskl
Copy link
Owner

giotiskl commented Feb 1, 2018

@jariz I agree with your thoughts.

Could you please set as the base to this and the future PR the develop branch?

Once merged and tested I will then prepare a new version and merge to master.

@jariz jariz changed the base branch from master to develop February 1, 2018 13:02
@jariz
Copy link
Contributor Author

jariz commented Feb 1, 2018

Cool, base changed.

@giotiskl giotiskl merged commit f9c9c03 into giotiskl:develop Feb 1, 2018
@giotiskl
Copy link
Owner

@jariz as an update, I am not aware if you got to start working on the rest of the changes you proposed, but as a matter of fact I implemented them myself, they're already on develop and coming in the next release tomorrow.

@jariz
Copy link
Contributor Author

jariz commented Feb 11, 2018

hiya @giotiskl, I haven't had the time yet myself, so thats awesome!

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

Successfully merging this pull request may close these issues.

2 participants