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 filters for Nodejs 10 #1772

Merged
1 commit merged into from Aug 19, 2018
Merged

Add filters for Nodejs 10 #1772

1 commit merged into from Aug 19, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 18, 2018

  1. Ref: release-post.js script should ignore 32-bit binaries for intel and sunos #1769
  2. Optimize the logic process for module.exports of download.js.

@ghost ghost requested review from MylesBorins and rvagg August 18, 2018 05:14
if (semver.satisfies(version, '< 1.0.0')) {
downloads = legacyDownloads
} else if (semver.satisfies(version, '< 8.0.0')) {
downloads = postMergeDownloads
} else if (semver.satisfies(version, '> 10.0.0')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be >= 10.0.0 or is it unrelated cause this will never run again for 10.0.0 ?

Copy link
Author

Choose a reason for hiding this comment

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

So are you sure that from the ref #1769, it should be >= 10.0.0? I'm not sure. I see it says Node 10+……?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, in #1769 it seems that 10.0.0 itself is not included. But the x86 binaries are not shipped for 10.0.0 too. https://nodejs.org/dist/v10.0.0/

Copy link
Author

Choose a reason for hiding this comment

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

OK, thanks!

Copy link
Contributor

@ckotzbauer ckotzbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost ghost requested a review from lpinca August 19, 2018 05:21
if (value.title === 'Linux 32-bit Binary' ||
value.title === 'SunOS 32-bit Binary') {
// Indicate that comingSoon shouldn't be shown
value.notShowComingSoon = true
Copy link
Member

Choose a reason for hiding this comment

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

Nit: wouldn't it be easier to use showComingSoon instead of it's negation? Then below we can do

if (binary.showComingSoon) {
  return `${binary.title}: *Coming soon*`
}
return binary.title

It's the same but I personally find it easier to grok.

Copy link
Author

@ghost ghost Aug 19, 2018

Choose a reason for hiding this comment

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

The reason is that we DON'T wanna show commingSoon, and only those versions >= 10.0.0 will meet the requirement. So most of downloadable items won't have this symbol (They are shown in default, ONLY with this symbol won't be shown).

If I change to yours, I'll have to add the property to the original postMergeDownloads JSON objects and set the default value to true. In fact there's no need doing that. Hope you understand :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I do.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you can use something like hideComingSoon as property name then.

Copy link
Author

Choose a reason for hiding this comment

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

OK. Thanks!
And we're still considering whether there's anything better or easier way……Maybe my 1st way is the best, or am I thinking of it a bit too complicated? :D

@targos
Copy link
Member

targos commented Aug 19, 2018

Will this still show both lines? They should not be there at all in the end.

@ghost
Copy link
Author

ghost commented Aug 19, 2018

@targos:What do you mean? Can you elebrate it more clear?

@targos
Copy link
Member

targos commented Aug 19, 2018

It seems to me that postMergeDownloads.filter should be used instead of postMergeDownloads.map. What we would like is to remove those lines from the blog posts entirely, not just remove the "Coming soon" part.

@ghost
Copy link
Author

ghost commented Aug 19, 2018

@targos:If I didn't take you wrong, I think what you mean is just directly adding the filter condition to the postMergeDownloads. My 1st version is just what you think now :) (If you are interested in this, please see:https://github.com/Maledong/nodejs.org/commit/52ddf53c2755fa1ad9463fa3cd70bec5552d0602).

However there's a question:In the urlOrComingSoon function, we couldn't check the version directly and easily. This means we don't know when to show (or not to show) Coming Soon.

And on the other hand, I'm NOT sure whether versions >10.0.0 also pass sendRequest's check? If it can pass, this means we should still show the link instead of filtering them all out. Because the author seems to say:

./scripts/release-post.js needs to not put "Coming soon" for 32-bit binaries for Linux x86 or "sunos" x86 for Node 10+...

IMO:If we can find the link through sendRequest in the urlOrComingSoon, still shows the link (So I don't think we should directly do a simple filtering).

Correct me if I take you wrong :)
And if it's right, I'll convert back to my 1st idea.

@ghost
Copy link
Author

ghost commented Aug 19, 2018

@targos:Maybe you are right, and I'm a little bit too complicated...
I'll fix that:)

1) Ref: #1769
2) Optimize the logic process for module.exports of download.js.i
@ghost
Copy link
Author

ghost commented Aug 19, 2018

@targos:Fixed that in a better way by filtering out unmeeted things.

@targos
Copy link
Member

targos commented Aug 19, 2018

Thanks. LGTM.

This pull request was closed.
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.

3 participants