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

🏗 SwG Release 0.1.22.152 #33329

Merged
merged 6 commits into from
Mar 18, 2021

Conversation

elijahsoria
Copy link
Contributor

Version: 0.1.22.152

Previous release: 0.1.22.151

@amp-owners-bot amp-owners-bot bot requested a review from chenshay March 17, 2021 20:58
@ChrisAntaki
Copy link
Contributor

Hey @rileyajones ! We updated the web-activities dependency to the latest version

@@ -955,7 +875,7 @@ class JsonLdParser {
possibleConfigs = [possibleConfigs];
}

for (let i = 0; i < possibleConfigs.length; i++) {
for (let i = 0; i < /** @type {Array} */ (possibleConfigs).length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicks (feel free to ignore):

  1. What is this an array of /** @type {Array<some type?>}*/?
  2. Making this a for in loop would remove the assignment required on like 879 and prevent i being overloaded be the inner loop on line 890.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll follow up in another PR!

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Page config parser: Improves typing subscriptions-project/swg-js#1564 updates the type to !Array{!JsonObject}

  2. Would a for-in loop remove the assignment? I know a for-of loop would, but currently we can only use those in tests and build-system stuff

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot about that, I pretty much just work in build-system. A forEach loop would also work.

Copy link
Contributor

Choose a reason for hiding this comment

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

find() might be better, since we want to terminate the loop after finding a match

forEach() loops only terminate on exceptions

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I hadn't really tried to understand what the code was doing. A find would be great. Maybe something like this?

const {config, productId} = possibleConfigs
.filter(config => this.checkType_.checkValue(config['@type'], ALLOWED_TYPES))
.map(config => {
  const partOfArray = this.valueArray_(config, 'isPartOf');
  if (!partOfArray) {
    return;
  }
  for (let i = 0; i < partOfArray.length; i++) {
    productId = this.discoverProductId_(partOfArray[i]);
    if (productId) {
      return {config, productId};
    }
  }
})
.find(Boolean) || {};

const isAccessibleForFree = this.bool_(
  this.singleValue_(config, 'isAccessibleForFree'),
  /* default */ true
);

return new PageConfig(productId, !isAccessibleForFree);

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

Successfully merging this pull request may close these issues.

5 participants