feat: added a fix for npmjs now returns readme in version instead of global…

[aremishevsky]

… metadata readme key

Type:

The following has been addressed in the PR:

• There is a related issue?
• Unit or Functional tests are included in the PR

Description:

[juanpicado]

The readmeDistTagsPriority seems to optimistic asumming all packages follow such approach. That's the point I was thinking last time. Just see the react dist-tags

 { latest: '16.3.1',
     next: '16.3.1',
     dev: '15.5.0-rc.2',
     canary: '16.4.0-alpha.0911da3' },

So, I'm not sure about it.

Also, we would need unit test for this.

[juanpicado]

I just checked the code. I'll test it later with real data.

[aremishevsky]

Hi @juanpicado

As an example of dist-tags i've took a dist-tasg list from verdaccio package.
I've just added "next","dev" and "canary" tags in this list.
Please note it will take the readme of the first found tag's version readme.

[juanpicado]

It seems to be fix https://github.com/npm/registry/issues/305

[juanpicado]

If you catch here readme you do not need to iterate.
let readme = latestVersion.readme || pkg.readme || '';

Logic should be like this

1. pkg.readme exist, return.
2. latestVersion.readme exist return
3. Iterate and return
4. No result, return emtpy string.

Please return readme whether is different than an empty string. I'd add || '' only as las resource.

Try to avoid iterations if are not need it.

[juanpicado]

getLatestReadme function requires a unit test in a new file. I suggest test/unit/latest.readme.spec.js

[aremishevsky]

Hi @juanpicado .
I've added some minor changes according to your review.
Could you please take a look?

[juanpicado]

@aremishevsky I'll do it after work :) Thanks for your changes. Please read this #677 (comment)
Your stuff should be feat: xxx

[aremishevsky]

@juanpicado
I've changed the last commit.
However I can't change previous commits using rebase command. Not sure that I will not revert some of your changes.
If it's critical to have all commits with "feat: xxx" in messages - i can re-commit my changes in a new forked repo. If not - you can just merge it.
Just let me know

[juanpicado]

[juanpicado]

I just noticed. Just above your code, there is a function called cleanUpReadme https://github.com/verdaccio/verdaccio/blob/master/src/lib/storage-utils.js#L66 which remove all readme from versions.

So, the result is gonna be always undefined.

So, I think

// In case of empty readme - trying to get ANY readme in the following order: 'next','beta','alpha','test','dev','canary'
  const readmeDistTagsPriority = [
    'next',
    'beta',
    'alpha',
    'test',
    'dev',
    'canary'];
  readmeDistTagsPriority.map(function(tag) {
    if(readme) {
      return readme;
    }
    const data = distTags[tag] ? versions[distTags[tag]] || {} : {};
    readme = _.trim(data.readme || readme);
  });

is a bit unecessary with the current approach, with local-storage we cannot afford save readme for each version otherwhise metadata will grow exponenxially, that's the main reason why is being removed.

About your commit would be something like this `"feat: return latest dis-tag readme whether readme is emtpy"

But you must remove the iteration, sorry I did not noticed that earlier.

[juanpicado]

Some lint is failing

/home/ubuntu/verdaccio/src/lib/storage-utils.js
  80:3  error  Expected space(s) after "if"  keyword-spacing
  92:5  error  Expected space(s) after "if"  keyword-spacing

[aremishevsky]

Hi @juanpicado

I've re-created a PR with a new single commit with a normal message: #687

As for iteration and cleanUpReadme calling - If you take a look closer - I've moved readme set before calling cleanup function - so it should be fine and it is needed.

As for lint errors - i've fixed them in new PR. Thanks for an advise.
Please review this PR: #687 after that please close this PR.

Thanks!

[aremishevsky]

Hi @juanpicado .
Did you have a chance to take a look on #687 PR?

Thanks

[juanpicado]

@aremishevsky give me some days, I'm un a hurry to finish Google Cloud plugin (before expiring my trial) and next week is the We Are Developers at Vienna 😓 ... I'll try to do it as soon as possible.

[juanpicado]

Thanks, done :) thanks for your patient I'll merge #687 and close this one.

[aremishevsky]

@juanpicado Thanks for review.
Could you please let me know once it would be published with a some next release version on npmjs?

[juanpicado]

Just after my dinner 🍽 😉

[juanpicado]

https://github.com/verdaccio/verdaccio/releases/tag/v3.0.0-beta.10

[aremishevsky]

@juanpicado do you have any plans to get rid of "beta" versions?
Maybe we should use some v3.0.0 version as well.

[juanpicado]

@aremishevsky beta is just a ⚠️ to encourage people to be careful, but, to be honest, is 100% stable and nothing big will change.

We have more than 370 test running+ 83% coverage + puppeteer. On 2.x not even half of it. Nothing will change in the next beta anyway. I want to finish first Google Cloud #473 and S3 #472 plugins to ensure the new Store API is 100% stable.

I hope we finish all stuff before ends this month. In short, you can use it, it's safe.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.