Allow packages without repository #36

zarianec · 2021-09-13T12:45:51Z

This is the follow-up of #30 and #31. This PR allows packages without a repository to be added to the packages.json with a null value.

resolve #30

scripts/update.js

zeke

Left a minor refactoring suggestion, but this looks good to me. I will defer to @eridal for a final approval of this.

test.js

zarianec · 2021-09-21T10:23:09Z

Updated cache to store packages without repository as well.

zeke · 2021-09-22T03:39:30Z

This looks good to me. What do you think, @eridal?

eridal · 2021-09-22T08:06:01Z

I have a few concerns with this proposal:

The metadata is not handled correctly, and over time the collected stats will deviate from the packages.

Currently the sum of repos in the metadata is exactly the number of packages:

const packages = require('./data/packages.json')
const metadata = require('./data/metadata.json')

Object.keys(packages).length === Object.values(metadata.repos).reduce((a, b) => a + b, 0)

There are a few changes required to get those numbers correctly:

in apply, when a change gets processed we need to update correctly the counter (+1/-1) depending if it is an insert/update/delete. My suggestion is to make extractUrl simply return the string url, so that later we can parse/validate the url and separate invalid/nulls urls.
in updateRepoStats we want to allow the counter to have impact when the url is null. My suggestion is to create a new entry in the repos, lets say unset, and then handle the special case where url === null.

This new feature is quite tricky as there are multiple cases to take into consideration, like a repo that used to have an url but now is null, or the other way around.

zarianec · 2021-09-22T08:46:00Z

@eridal thanks for your review and suggestions. But I didn't get this one:

My suggestion is to make extractUrl simply return the string URL

Isn't it already return a string URL and we parse/validate it later? I don't see what do you want to change in it.

zarianec · 2021-09-22T10:37:59Z

It could be easier to implement this if we have tests for it. I already have tests that cover the update process and caching but as I mentioned before it requires some significant changes to the update.js script.

The biggest issue I had is the self-invoking anonymous function which makes it almost impossible to test because of process.exit

Currently, I use a workaround to prevent the script from immediate execution like:

module.exports = update

if (process.env.NODE_ENV !== 'test') {
  /**
   * Main
   */
  update().then(
    () => {
      console.log(metadata)
      process.exit(0)
    },
    (err) => {
      console.error(err)
      process.exit(1)
    }
  )
}

The better way IMO is to split it and move all the update logic to a separate file. update.js should only call this logic and finish the process.

@eridal I can push my tests here and we will work on edge cases/new requirements as it seems this PR will never get finished without it.

zeke · 2021-09-22T15:33:22Z

push my tests here and we will work on edge cases/new requirements

👍🏼

# Conflicts: # package-lock.json # package.json # scripts/stats.js

zarianec · 2021-09-22T16:19:18Z

@zeke @eridal pushed tests. I see we already have a lib folder. I think it's a good idea to split the update.js and move it to lib completely. Scripts in scripts will simply use it and handle the results.

- convo: #30 - ideas: #36

eridal · 2021-09-25T19:07:55Z

@zarianec I owe you an apology.

You have been putting a good amount of efforts into this feature. It was easy for me to quickly hack a solution, submit that PR, but doing so disregarded your work in the process. I'm sorry about it and I'm gonna take action to fix that.

This is open source, the code I wrote is from the community, and the more people involved the better; so I'd like you to take over this feature and push it to go live. So I'm gonna go ahead and close my PR, please use it as a source of inspiration --or not, you decide :)

Some ideas behind this PR are super good. My only suggestion is try not to change too much the code, but rather make smaller, incremental changes. If changes are not part of this feature, please submit another PR only for that particular change (ie: mocha update) so that this PR will stay minimal while also making progress. Also it'd be nice if you can execute a full-history reprocess, so more people knows how to do so.

Please accept my apologies, I am totally confident you will be able to deliver this feature!

zeke · 2021-09-27T04:54:42Z

Thanks for chiming in, @eridal. 💛

My only suggestion is try not to change too much the code, but rather make smaller, incremental changes.

Totally agree.

I'd like you to take over this feature and push it to go live.

Yeah let's do it while the fire is still hot. @zarianec is this PR ready to ship?

# Conflicts: # package-lock.json # package.json

zarianec · 2021-09-28T22:14:53Z

@eridal thank you for your understanding, I really appreciate it. Some of the changes in your PR are really helpful and I moved it there (like handling invalid packages and better stats in the readme).

My only suggestion is try not to change too much the code, but rather make smaller, incremental changes.

I didn't want to refactor the tests as part of this PR and planned to open a new one after we finish with this. So I revert it for now and add the metadata tests you created in #39

Also it'd be nice if you can execute a full-history reprocess, so more people knows how to do so.

I did it already many times and must say that the caching you implemented is amazing - it saved me a LOT of time.

is this PR ready to ship?

Unfortunately no. I ran the full rebuild of the index and the metadata test suddenly failed:

1) metadata repos should match repos:

      AssertionError: expected 1050411 to equal 1050413
      + expected - actual

      -1050411
      +1050413

  2) metadata repos should match urls:

      AssertionError: expected 712688 to equal 712690
      + expected - actual

      -712688
      +712690

It took me some time to figure out why 2 packages are missing and the reason is pretty interesting. There is an NPM package called constructor and packages store is a generic javascript object so doing this:

all-the-package-repos/scripts/update.js

Line 218 in 5dd54c5

const curr = packages[name]

Is the same as:

let foo = repos['constructor'] // we simply grab the object's constructor function
console.log(foo) // [Function: Object]

The better (and safer) way to handle the arbitrary list of key-value pairs in javascript is Map so I'm thinking of using it to store the packages list even considering performance overhead.

const packages = new Map(fs.existsSync(files.packages) ? Object.entries(require(files.packages)) : [])

zeke · 2021-09-29T17:05:54Z

scripts/update.js

@@ -427,7 +427,7 @@ const writeChanges = (deferred) => {
  fs.writeFileSync(files.metadata, toJson(metadata))

  if (batch.found > 0) {
-    fs.writeFileSync(files.packages, toJson(sort(packages)))
+    fs.writeFileSync(files.packages, toJson(sort(Object.fromEntries(packages))))


So you're using a Map when collecting data then turning it into an Object when finished? That makes sense to me. 👍🏼

Yes, basically this is it. I updated the sort function so it will work with the Map as an input instead of an object - no need to convert the map to an object before sorting it.

eridal · 2021-10-01T18:08:04Z

scripts/update.js

-    sorted[key] = object[key]
-    delete object[key]
+    sorted[key] = packagesMap.get(key)
+    delete packagesMap.delete(key)


there's an extra delete keyword here

fixed, thanks

eridal · 2021-10-01T18:10:02Z

scripts/update.js


  if (change.deleted) {
-    if (typeof curr === 'string') {
+    if (typeof curr !== 'undefined') {


You can take advantage of the map: package.has(name)

It looked strange to me to check with packages.has() and use the curr variable then. And replacing curr with packages.get() everywhere looked too wordy.

eridal · 2021-10-01T18:11:33Z

Hey, this is looking great!

I wonder if it makes sense that this package exposes the Map to its consumers, so that they wont have the same issue that you found here.

zarianec · 2021-10-01T19:01:30Z

The issue with a constructor key happens only when you try to get the value if it wasn't set before. Since the final packages list has it already, it will work fine until the package will get deleted from the NPM 🙂

So exposing the Map instead of Object makes sense but we should consider memory overhead for Object->Map conversion.

Or we might expose the Object without a prototype (Object.create(null)) to keep the interface the same and make migration easier. But it will add performance overhead anyway.

eridal · 2021-10-02T17:28:43Z

lib/stats-tpl.js

+      With repository | ${notNull} | ${perc(notNull)}%
+      Null repository | ${areNull} | ${perc(areNull)}%
+      **Total** | ${total} | ${perc(total)}%
+


Nit: unnecessary whitespace here

It's required here to split two tables. See how it looks without it:

Packages Count Percentage

With repository 1004148 64.84%

Null repository 544609 35.16%

Total 1548757 100.00%

Providers Count Percentage

:-------- -----: ----------:

GitHub 986341 63.69%

GitLab 3624 0.23%

Bitbucket 1148 0.07%

Others 13035 0.84%

Total 1004148 64.84%

eridal · 2021-10-02T17:29:52Z

hostnames.js

@@ -3,7 +3,7 @@
 const URL = require('url')
 const countValues = require('count-array-values')
 const urls = Object.values(require('.'))
-const hostnames = urls.map(url => URL.parse(url).hostname.replace(/^www\./i, ''))
+const hostnames = urls.filter(u => u).map(url => URL.parse(url).hostname.replace(/^www\./i, ''))


Can we use .filter(Boolean)?

Yes, fixed.

eridal · 2021-10-02T17:38:17Z

test/repos.js

+    expect(urls.some(url => isUrl(url)), 'should contain packages with repos').to.equal(true)
+    expect(urls.some(url => url === null), 'should contain packages without repos').to.equal(true)


This is expecting at least a null, which will not be the case if this is merged into current master. I'd say use the test to expect what we know it might be there, either a null or an url..

for (const url of Object.values(repos)) { expect(url === null || isUrl(url), url).to.equal(true) }

Also nothing prevents the packages to be all set, and while super low probability there could be no-null at some point :))

which will not be the case if this is merged into current master

Agree, it will fail if we merge it without updating the data. On the other hand master with these changes and old index should be considered broken so probably it's fine.

while super low probability there could be no-null at some point

The probability of that is so low that I would rather consider it a bug if our index doesn't have any null value. It will prevent the situation when packages without a repository are missing in the index because of a bug in the code.

eridal · 2021-10-02T17:57:24Z

Leaving aside those small comments, this looks ready to ship. Great work @zarianec!

The Map can be delayed as consumers of this package will always read whatever is exported as part of the object, so the prototype chain make things like constructor or hasOwnProperty safer for them.

const foo = {}
console.log(typeof foo.hasOwnProperty) // function
foo.hasOwnProperty = 'nope'
console.log(typeof foo.hasOwnProperty) // string

Let's talk about release. This a breaking change for others so we need a major version.

zeke · 2021-10-03T00:28:53Z

🍿 Great to see the progress here!

zarianec · 2021-10-07T09:38:36Z

The release plan should be the same as @eridal described in the #32 (comment) with one additional step - update the major version of the package.

zarianec · 2021-10-07T09:40:08Z

@eridal @zeke I can do it as soon as this PR gets approved by everyone and we decide that it's ready to be shipped.

zeke · 2021-10-07T16:59:05Z

Sounds good. Wanna have a zoom call soon to pair on the release?

zarianec · 2021-10-21T18:18:18Z

@zeke sorry, I had a lot of stuff to do last week. I run the update script locally to have metadata and packages ready. Probably it will run all night. I will push updated data to this PR tomorrow and it will be ready for merge.

So the plan is like this:

push the updated data
disable actions (just in case)
merge it to master
update the major version
enable actions

Wanna have a zoom call soon to pair on the release?

good idea but could be tricky since I'm in a CEST timezone. DM me in Twitter so we could find the best time slot.

zeke · 2021-10-21T21:49:19Z

DM me in Twitter

Followed you, but don't see an option to DM just yet. You DM me. :)

zeke · 2021-10-25T16:10:27Z

Thanks for the updates @zarianec. I'll try to get this released in the new few days.

I see that there's a conflict. Is that expected? Should I force the changes from your updated version of metadata.json here?

zarianec · 2021-10-26T17:07:59Z

Yes, it's safe to force the changes from metadata.json. They will get updated on the next action run.

zarianec · 2021-11-23T20:29:07Z

@zeke I just checked if I have all required permissions to release this PR. Looks like I'm good to go so I'm going to release this PR tomorrow (need to rebuild index from scratch to eliminate NPM's private packages leakage)

zeke · 2021-11-23T20:30:27Z

Thanks @zarianec

Sorry I totally dropped the ball on this. 🙈

allow packages without repository

4763b1b

zarianec requested a review from zeke September 13, 2021 12:52

eridal reviewed Sep 15, 2021

View reviewed changes

scripts/update.js Outdated Show resolved Hide resolved

eridal reviewed Sep 15, 2021

View reviewed changes

scripts/update.js Outdated Show resolved Hide resolved

count invalid packages

ecbdf01

zeke reviewed Sep 16, 2021

View reviewed changes

scripts/update.js Outdated Show resolved Hide resolved

zeke approved these changes Sep 16, 2021

View reviewed changes

test.js Outdated Show resolved Hide resolved

zarianec added 4 commits September 17, 2021 12:51

update tests

97771da

Merge remote-tracking branch 'origin/master' into null-repos

7cc3e99

cache packages without repository

68088f0

Merge remote-tracking branch 'origin/master' into null-repos

f1c1cc7

zarianec added 2 commits September 22, 2021 17:59

add tests

ac5fdb7

Merge remote-tracking branch 'origin/master' into null-repos

92ae588

# Conflicts: # package-lock.json # package.json # scripts/stats.js

fix bug with deleting of packages

843eb97

eridal added a commit that referenced this pull request Sep 24, 2021

Allow null as repository url

16ad599

- convo: #30 - ideas: #36

eridal mentioned this pull request Sep 24, 2021

Allow null as repository url #39

Closed

eridal added a commit that referenced this pull request Sep 24, 2021

Allow null as repository url

f3a9260

- convo: #30 - ideas: #36

eridal added a commit that referenced this pull request Sep 24, 2021

Allow null as repository url

aae5eab

- convo: #30 - ideas: #36

zarianec added 2 commits September 28, 2021 13:39

revert tests and apply changes from #39

6911aa5

Merge remote-tracking branch 'origin/master' into null-repos

85df95b

# Conflicts: # package-lock.json # package.json

zeke reviewed Sep 29, 2021

View reviewed changes

update sorting

a1e9637

eridal reviewed Oct 1, 2021

View reviewed changes

fix extra delete keyword

608c580

eridal reviewed Oct 2, 2021

View reviewed changes

fix hostnames filter

5512572

Merge remote-tracking branch 'origin/master' into null-repos

5283201

zarianec added 2 commits October 26, 2021 19:04

updated metadata

f946128

Merge remote-tracking branch 'origin/master' into null-repos

5847d6e

zarianec mentioned this pull request Nov 23, 2021

Is this project not maintained anymore? nice-registry/all-the-package-names#25

Closed

zarianec added 3 commits November 25, 2021 13:23

Merge remote-tracking branch 'origin/master' into null-repos

801d7b5

update metadata

f772b92

ignore .idea

068f97f

zarianec merged commit 8bf0fdc into nice-registry:master Nov 25, 2021

Packages	Count	Percentage
With repository	1004148	64.84%
Null repository	544609	35.16%
Total	1548757	100.00%
Providers	Count	Percentage
:--------	-----:	----------:
GitHub	986341	63.69%
GitLab	3624	0.23%
Bitbucket	1148	0.07%
Others	13035	0.84%
Total	1004148	64.84%

		expect(urls.some(url => isUrl(url)), 'should contain packages with repos').to.equal(true)
		expect(urls.some(url => url === null), 'should contain packages without repos').to.equal(true)

Allow packages without repository #36

Allow packages without repository #36

Conversation

zarianec commented Sep 13, 2021

zeke left a comment

Choose a reason for hiding this comment

zarianec commented Sep 21, 2021 • edited Loading

zeke commented Sep 22, 2021

eridal commented Sep 22, 2021

zarianec commented Sep 22, 2021

zarianec commented Sep 22, 2021

zeke commented Sep 22, 2021

zarianec commented Sep 22, 2021

eridal commented Sep 25, 2021

zeke commented Sep 27, 2021

zarianec commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eridal commented Oct 1, 2021

zarianec commented Oct 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eridal commented Oct 2, 2021 • edited Loading

zeke commented Oct 3, 2021

zarianec commented Oct 7, 2021

zarianec commented Oct 7, 2021

zeke commented Oct 7, 2021

zarianec commented Oct 21, 2021

zeke commented Oct 21, 2021

zeke commented Oct 25, 2021

zarianec commented Oct 26, 2021

zarianec commented Nov 23, 2021

zeke commented Nov 23, 2021

zarianec commented Sep 21, 2021 •

edited

Loading

eridal commented Oct 2, 2021 •

edited

Loading