Skip to content

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Jun 9, 2020

Partial for #28737 #14357

Closes #21251
Closes #14357
Closes #28737

@Enriqe Enriqe requested a review from jridgewell June 9, 2020 21:36
@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 9, 2020

@jridgewell can you approve for bundle size?

@newmuis
Copy link
Contributor

newmuis commented Jun 10, 2020

Has this been tested to ensure the basic functionality still works?

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 10, 2020

Hmm not sure how to test this... In my local server I tried running gulp --compiled and loading a document containing amp-story-0.1, hoping it would load amp-story-1.0 instead, but that didn't work... Could you help us here @rsimha ?

@rsimha
Copy link
Contributor

rsimha commented Jun 10, 2020

Hmm not sure how to test this... In my local server I tried running gulp --compiled and loading a document containing amp-story-0.1, hoping it would load amp-story-1.0 instead, but that didn't work... Could you help us here @rsimha ?

I suspect this PR is incomplete. In addition to aliasing 0.1 to 1.0, don't you have to delete 0.1? See amp-sticky-ad for an example.

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 10, 2020

Thanks @rsimha , it looks like we were still generating the build targets for amp-story-0.1, after deleting that I can confirm that loading a document using amp-story-0.1 loads the same JS as amp-story-1.0 and basic functionality still works @newmuis.

Re-requesting approvals.

@Enriqe Enriqe requested review from rsimha and gmajoulet June 10, 2020 21:50
@rsimha
Copy link
Contributor

rsimha commented Jun 10, 2020

Don’t you have to delete the source code too?

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 10, 2020

I was planning on doing that on a separate PR, if that's ok. I'd like this one to solely focus on the aliasing part as much as possible.

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 11, 2020

Actually I might have to delete the source code as part of this PR since pre-submit checks are complaining about not finding the deleted bundles.

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

I'm not sure why you need a bundle size approval. What's changed in v0?

@Enriqe Enriqe force-pushed the alias-story-0.1 branch from ce536af to 8c237e1 Compare June 12, 2020 19:14
@amp-owners-bot
Copy link

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

extensions/amp-story/0.1/_locales/default.js
extensions/amp-story/0.1/_locales/en.js
extensions/amp-story/0.1/amp-story-base-layer.js
extensions/amp-story/0.1/amp-story-bookend.css
extensions/amp-story/0.1/amp-story-bookend.js
extensions/amp-story/0.1/amp-story-consent.css
extensions/amp-story/0.1/amp-story-consent.js
extensions/amp-story/0.1/amp-story-cta-layer.js
extensions/amp-story/0.1/amp-story-desktop-user-overridable.css
extensions/amp-story/0.1/amp-story-desktop.css
extensions/amp-story/0.1/amp-story-grid-layer.js
extensions/amp-story/0.1/amp-story-hint.css
extensions/amp-story/0.1/amp-story-hint.js
extensions/amp-story/0.1/amp-story-info-dialog.css
extensions/amp-story/0.1/amp-story-info-dialog.js
extensions/amp-story/0.1/amp-story-localization-service.js
extensions/amp-story/0.1/amp-story-page.js
extensions/amp-story/0.1/amp-story-render-service.js
extensions/amp-story/0.1/amp-story-request-service.js
extensions/amp-story/0.1/amp-story-share-menu.css
extensions/amp-story/0.1/amp-story-share-menu.js
extensions/amp-story/0.1/amp-story-share.css
extensions/amp-story/0.1/amp-story-share.js
extensions/amp-story/0.1/amp-story-store-service.js
extensions/amp-story/0.1/amp-story-system-layer.css
extensions/amp-story/0.1/amp-story-system-layer.js
extensions/amp-story/0.1/amp-story-unsupported-browser-layer.css
extensions/amp-story/0.1/amp-story-unsupported-browser-layer.js
extensions/amp-story/0.1/amp-story-user-overridable.css
extensions/amp-story/0.1/amp-story-viewport-warning-layer.css
extensions/amp-story/0.1/amp-story-viewport-warning-layer.js
extensions/amp-story/0.1/amp-story.css
extensions/amp-story/0.1/amp-story.js
extensions/amp-story/0.1/analytics.js
extensions/amp-story/0.1/animation-presets-utils.js
extensions/amp-story/0.1/animation-presets.js
extensions/amp-story/0.1/animation-types.js
extensions/amp-story/0.1/animation.js
extensions/amp-story/0.1/audio.js
extensions/amp-story/0.1/background.js
extensions/amp-story/0.1/default-media.js
extensions/amp-story/0.1/development-ui.js
extensions/amp-story/0.1/embed-mode.js
extensions/amp-story/0.1/events.js
extensions/amp-story/0.1/jsonld.js
extensions/amp-story/0.1/loading-spinner.js
extensions/amp-story/0.1/logging.js
extensions/amp-story/0.1/media-pool.js
extensions/amp-story/0.1/media-pool.md
extensions/amp-story/0.1/media-tasks.js
extensions/amp-story/0.1/navigation-state.js
extensions/amp-story/0.1/origin-allowlist.js
extensions/amp-story/0.1/page-advancement.js
extensions/amp-story/0.1/page-element.js
extensions/amp-story/0.1/pagination-buttons.js
extensions/amp-story/0.1/progress-bar.js
extensions/amp-story/0.1/related-articles.js
extensions/amp-story/0.1/simple-template.js
extensions/amp-story/0.1/sources.js
extensions/amp-story/0.1/test/test-amp-story-consent.js
extensions/amp-story/0.1/test/test-amp-story-cta-layer.js
extensions/amp-story/0.1/test/test-amp-story-hint.js
extensions/amp-story/0.1/test/test-amp-story-info-dialog.js
extensions/amp-story/0.1/test/test-amp-story-request-service.js
extensions/amp-story/0.1/test/test-amp-story-share-menu.js
extensions/amp-story/0.1/test/test-amp-story-store-service.js
extensions/amp-story/0.1/test/test-amp-story-system-layer.js
extensions/amp-story/0.1/test/test-amp-story.js
extensions/amp-story/0.1/test/test-analytics.js
extensions/amp-story/0.1/test/test-full-bleed-animations.js
extensions/amp-story/0.1/test/test-loading-spinner.js
extensions/amp-story/0.1/test/test-media-pool.js
extensions/amp-story/0.1/test/test-media-tasks.js
extensions/amp-story/0.1/test/test-navigation-state.js
extensions/amp-story/0.1/test/test-origin-allowlist.js
extensions/amp-story/0.1/test/test-utils.js
extensions/amp-story/0.1/test/test-variable-service.js
extensions/amp-story/0.1/test/validator-amp-story-animations-error.html
extensions/amp-story/0.1/test/validator-amp-story-animations-error.out
extensions/amp-story/0.1/test/validator-amp-story-animations.html
extensions/amp-story/0.1/test/validator-amp-story-animations.out
extensions/amp-story/0.1/test/validator-amp-story-consent-geo.html
extensions/amp-story/0.1/test/validator-amp-story-consent-geo.out
extensions/amp-story/0.1/test/validator-amp-story-consent.html
extensions/amp-story/0.1/test/validator-amp-story-consent.out
extensions/amp-story/0.1/test/validator-amp-story-cta-layer-error.html
extensions/amp-story/0.1/test/validator-amp-story-cta-layer-error.out
extensions/amp-story/0.1/test/validator-amp-story-cta-layer.html
extensions/amp-story/0.1/test/validator-amp-story-cta-layer.out
extensions/amp-story/0.1/test/validator-amp-story-deprecated.html
extensions/amp-story/0.1/test/validator-amp-story-deprecated.out
extensions/amp-story/0.1/test/validator-amp-story-error.html
extensions/amp-story/0.1/test/validator-amp-story-error.out
extensions/amp-story/0.1/test/validator-amp-story-reference-point.html
extensions/amp-story/0.1/test/validator-amp-story-reference-point.out
extensions/amp-story/0.1/test/validator-amp-story-video-error.html
extensions/amp-story/0.1/test/validator-amp-story-video-error.out
extensions/amp-story/0.1/test/validator-amp-story.html
extensions/amp-story/0.1/test/validator-amp-story.out
extensions/amp-story/0.1/test/validator-empty-story.html
extensions/amp-story/0.1/test/validator-empty-story.out
extensions/amp-story/0.1/toast.js
extensions/amp-story/0.1/utils.js
extensions/amp-story/0.1/variable-service.js

@Enriqe Enriqe force-pushed the alias-story-0.1 branch from 8c237e1 to e468e9b Compare June 12, 2020 19:15
@Enriqe Enriqe changed the title 🔄 Alias amp-story 0.1 to 1.0 🔄🗑🚮 Alias amp-story 0.1 to 1.0 and delete code Jun 12, 2020
@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 12, 2020

Included the deleted source code changes, PTAL :)

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 12, 2020

@jridgewell not sure why it says that dist/v0/amp-story-0.1.js bundle has increased since it's being deleted.

image

EDIT: it looks like it went away

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

tenor (10)
tenor (9)
giphy (15)

@Enriqe Enriqe requested a review from jridgewell June 12, 2020 19:45
@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 12, 2020

@jridgewell requesting approval for src/services.js

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 12, 2020

need approval from @ampproject/wg-caching

@Enriqe Enriqe merged commit 0ccf6f4 into ampproject:master Jun 12, 2020
mszylkowski pushed a commit to mszylkowski/amphtml that referenced this pull request Jun 17, 2020
* alias amp-story 0.1 to 1.0

* delete generation of 0.1 bundle

* delete 0.1 code

* deps, more code
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.

Delete amp-story-0.1 code I2I: amp-story version 0.1 deprecation Fork amp-story to 1.0
8 participants