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

add IndexedDBShim@2.2.1 w/ git auto-update #10443

Merged
merged 1 commit into from Feb 8, 2017
Merged

add IndexedDBShim@2.2.1 w/ git auto-update #10443

merged 1 commit into from Feb 8, 2017

Conversation

kaocy
Copy link
Contributor

@kaocy kaocy commented Jan 28, 2017

close #10261, cc @brettz9

Pull request for issue: #10261
Related issue(s): # #

Checklist for Pull request or lib adding request issue follows the conventions.

Note that if you are using a distribution purpose repository/package, please also provide the url and other related info like popularity of the source code repo/package.

Profile of the lib

Essential checklist

  • I'm the author of this library
    • I would like to add link to the page of this library on CDNJS on website / readme
  • This lib was not found on cdnjs repo
  • No already exist / duplicated issue and PR
  • The lib has notable popularity
    • More than 100 [Stars / Watchers / Forks] on [GitHub / Bitbucket]
    • More than 500 downloads stats per month on npm registry
  • Project has public repository on famous online hosting platform (or been hosted on npm)

Auto-update checklist

  • Has valid tags for each versions (for git auto-update)
  • Auto-update setup
  • Auto-update target/source is valid.
  • Auto-update filemap is correct.

Git commit checklist

  • The first line of commit message is less then 50 chars, be clean and clear, easy to understand.
  • The parent of the commit(s) in the PR is not old than 3 days.
  • Pull request is sending from a non-master branch with meaningful name.
  • Separate unrelated changes into different commits.
  • Use rebase to squash/fixup dummy/unnecessary commits into only one commit.
  • Close corresponding issue in commit message
  • Mention related issue(s), people in commit message, comment.

@kaocy
Copy link
Contributor Author

kaocy commented Jan 28, 2017

@x09326 Please review this PR. Thanks!

Copy link
Contributor

@maruilian11 maruilian11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"polyfill",
"websql"
],
"license": "Apache-2.0 or MIT",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use an array.

Copy link

@brettz9 brettz9 Jan 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you're doing internally, but an array is not preferred anymore for package.json: https://docs.npmjs.com/files/package.json#license

Some old packages used license objects or a "licenses" property containing an array of license objects:

Those styles are now deprecated. Instead, use SPDX expressions...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other files are in master branch, not in the latest version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maruilian11 I have modified the part of license. Please review it again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @brettz9, we use SPDX expressions, too. Just keep the same structure when we meet multi-license lib in cdnjs.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, we've since added parentheses for the license field as per the SPDX specs (on 1/27: )...

@maruilian11
Copy link
Contributor

Please don't mention close issue and cc the author in comments next time.

@maruilian11
Copy link
Contributor

LGTM

@maruilian11
Copy link
Contributor

@x09326 @kennynaoh Please help me make the double check, thanks!

@x09326
Copy link
Contributor

x09326 commented Feb 6, 2017

@lcd78706
Please rebase to the latest master branch, thanks.

@kaocy
Copy link
Contributor Author

kaocy commented Feb 6, 2017

@x09326 I have rebased it to the latest master branch. Please review it again. Thanks!

Copy link
Contributor

@kennynaoh kennynaoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@x09326
Copy link
Contributor

x09326 commented Feb 7, 2017

LGTM
ping @PeterDaveHello

Copy link
Contributor

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use its repo name IndexedDBShim as the easier to be understood name, any reason you need to change it to indexeddbshim?

@kaocy
Copy link
Contributor Author

kaocy commented Feb 7, 2017

@PeterDaveHello
The name written in package.json is originally indexeddbshim.
I just keep it and didn't change it.

@PeterDaveHello
Copy link
Contributor

@lcd78706 We don't decide the library name by a single factor, repository name, readability should also be considered, let's update it.

@kaocy
Copy link
Contributor Author

kaocy commented Feb 7, 2017

@x09326
I have modified it. Please review it again. Thanks!

@kaocy kaocy changed the title add indexeddbshim@2.2.1 w/ git auto-update add IndexedDBShim@2.2.1 w/ git auto-update Feb 7, 2017
@x09326
Copy link
Contributor

x09326 commented Feb 8, 2017

LGTM
@cdnjs/intern2 please help me review it, thanks.

@PeterDaveHello PeterDaveHello merged commit aaf6fc1 into cdnjs:master Feb 8, 2017
PeterDaveHello added a commit that referenced this pull request Feb 8, 2017
@kaocy kaocy deleted the indexeddbshim branch February 12, 2017 02:09
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.

[Request] Add indexeddbshim
6 participants