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

♻️ Extract json helpers to core/types/object/json #34367

Merged
merged 15 commits into from
May 14, 2021

Conversation

rcebulko
Copy link
Contributor

@rcebulko rcebulko commented May 13, 2021

This is the diff to review: 2179341...54ada92
Everything else is just updating import statements.

Part of #32693

Also they pass type-checking now

@amp-owners-bot
Copy link

amp-owners-bot bot commented May 13, 2021

Hey @erwinmombay! These files were changed:

build-system/eslint-rules/json-configuration.js
build-system/eslint-rules/no-has-own-property-method.js

Hey @jridgewell! These files were changed:

build-system/eslint-rules/json-configuration.js
build-system/eslint-rules/no-has-own-property-method.js
src/core/data-structures/finite-state-machine.js
src/core/data-structures/promise.js
src/core/data-structures/signals.js
src/core/error.js
src/core/types/object/index.js
src/core/types/object/json.extern.js
src/core/types/object/json.js

Hey @Jiaming-X! These files were changed:

extensions/amp-ad-network-adsense-impl/0.1/responsive-state.js

Hey @jeffkaufman! These files were changed:

extensions/amp-ad-network-adsense-impl/0.1/responsive-state.js
extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js

Hey @alanorozco! These files were changed:

extensions/amp-connatix-player/0.1/amp-connatix-player.js

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story-panning-media/0.1/amp-story-panning-media.js
extensions/amp-story/1.0/amp-story-access.js
extensions/amp-story/1.0/amp-story-consent.js
extensions/amp-story/1.0/amp-story-store-service.js
extensions/amp-story/1.0/history.js
extensions/amp-story/1.0/jsonld.js
src/localized-strings.js

Hey @processprocess! These files were changed:

extensions/amp-story-panning-media/0.1/amp-story-panning-media.js

Hey @Enriqe! These files were changed:

extensions/amp-story/1.0/amp-story-access.js
extensions/amp-story/1.0/amp-story-consent.js
extensions/amp-story/1.0/amp-story-store-service.js
extensions/amp-story/1.0/history.js
extensions/amp-story/1.0/jsonld.js
src/amp-story-player/amp-story-player-impl.js
src/localized-strings.js

@@ -0,0 +1,351 @@
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was moved from test/unit/core/types/test-object.js

@@ -1,5 +1,5 @@
/**
* Copyright 2015 The AMP HTML Authors. All Rights Reserved.
* Copyright 2021 The AMP HTML Authors. All Rights Reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

GH doesn't show this, but this file was moved to src/core/object/types/json.js (which has the 2015 copyright date) and this file is "new"

Copy link
Member

@danielrozenberg danielrozenberg left a comment

Choose a reason for hiding this comment

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

Approval for build-system/*

@rcebulko rcebulko enabled auto-merge (squash) May 13, 2021 18:20
src/core/types/object/json.js Outdated Show resolved Hide resolved
src/json.js Show resolved Hide resolved
@rcebulko rcebulko disabled auto-merge May 13, 2021 20:07
This was referenced Nov 5, 2021
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.

6 participants