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.

Sign up for GitHub

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

Jump to bottom

🚮 Assist.js: Remove assistjs extension and all relevant libraries. #35023

Merged
merged 15 commits into from
Jun 30, 2021
Merged

🚮 Assist.js: Remove assistjs extension and all relevant libraries. #35023

merged 15 commits into from
Jun 30, 2021

Conversation

ychsieh
Copy link
Contributor

@ychsieh ychsieh commented Jun 24, 2021 •
edited
Loading

Assist.js AMP integration I2I issue: #30631(closing)

@samouri
/cc @ianba

@ychsieh
Add runtime service to handle execute tasks like adding ports to the …
74f0a03 
…network.

- Add entry points to wrapper such as creating PortOperator and adding ports.
- Update compile script since it can perform as normal Closure compiler.
@ychsieh
Fix lint errors.
cea0dbb
@ychsieh
Remove externs.js since the compiled code with it would keep 'goog' h…
2414afd 
…ence violate JSC_CANNOT_HAVE_MODULE_VAR_NAMED_GOOG.
@ychsieh
Construct channel with FrameService iframe and add it to the network.
d54c4a6 
- Add DeferredChannel to the network first and resolve it after the real channel is constructed.
- Ensure FrameService port is always added before any widget initialization
@ychsieh
Report available widgets to FrameService so that it can request widge…
5a54ad8 
…t ports to listen on.

- Add protocol buffer library to generate FrameService protocol
- Report widget by sending messages to FrameService's RespondingChannel.
- Add necessary endpoints to Closure custom API.
@ychsieh
Move proto library to third_party folder.
39e69ee 
- Move entire proto folder to third_party folder
- Update whitelist and source globs for new third_party library
- Add exports file so that 'require' can be avoided in the extension
@ychsieh
Fix source glob error.
9710fb7
@ychsieh
Fix source glob error ampproject#2.
84ca57a
@ychsieh
Update protoc from protobug Github repo.
a890061
@ychsieh
Remove assistjs extension and all relevant libraries.
5ddf0de
@amp-owners-bot amp-owners-bot bot requested a review from jridgewell June 24, 2021 18:20
@amp-owners-bot
Copy link

amp-owners-bot bot commented Jun 24, 2021 •
edited by ampprojectbot
Loading

Hey @rsimha! These files were changed:

build-system/compile/compile.js

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-google-assistant-assistjs/0.1/test/validator-amp-google-assistant-assistjs.html
extensions/amp-google-assistant-assistjs/0.1/test/validator-amp-google-assistant-assistjs.out
extensions/amp-google-assistant-assistjs/validator-amp-google-assistant-assistjs.protoascii

@ychsieh ychsieh enabled auto-merge (squash) June 24, 2021 18:22
@honeybadgerdontcare
Copy link
Contributor

Is there an I2R this should reference? Has any analysis been made on how many documents this may affect?

@ychsieh
Copy link
Contributor Author

ychsieh commented Jun 24, 2021

There shouldn't be analysis needed since this extension is not used anywhere.

@honeybadgerdontcare
Copy link
Contributor

There shouldn't be analysis needed since this extension is not used anywhere.

How have you come to that conclusion without any analysis?

What is the I2R for this?

@ychsieh
Copy link
Contributor Author

ychsieh commented Jun 24, 2021

Just created: #35024. Thanks for the reminder.

@samouri
Copy link
Member

samouri commented Jun 24, 2021 •
edited
Loading

@honeybadgerdontcare: this was a WIP extension that was completed. It was never functional. At some point during implementation the team decided to end the effort, so this is just a cleanup.

@honeybadgerdontcare
Copy link
Contributor

@honeybadgerdontcare: this was a WIP extension that was completed. It was never functional. At some point during implementation the team decided to end the effort, so this is just a cleanup.

Thank you for clarifying. Regardless this was something allowed and is now being disallowed. It should be verified that documents aren't using it even if it wasn't encouraged. I haven't seen anything stating this is not used in the open web.

@ychsieh
Copy link
Contributor Author

ychsieh commented Jun 25, 2021

Thanks for the kind scrutiny. This extension has always been under development and never entered into a usable stage, meaning all developers that tried to use it would either crashed their pages or found it non-responding. As a result, I don't think anyone is using it for serious purpose. Does that sound reasonable?

@honeybadgerdontcare
Copy link
Contributor

@samouri for review

honeybadgerdontcare
honeybadgerdontcare approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

approved for validator files

@ychsieh ychsieh merged commit fec8b59 into ampproject:main Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsimha rsimha rsimha left review comments

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare approved these changes

@jridgewell jridgewell jridgewell approved these changes

@samouri samouri Awaiting requested review from samouri

Assignees
No one assigned
Labels
WG: caching
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ychsieh @honeybadgerdontcare @samouri @jridgewell @rsimha