Skip to content

🚀 Compile v0/extensions in strict mode#29929

Merged
samouri merged 7 commits intoampproject:masterfrom
samouri:launchstrict
Aug 26, 2020
Merged

🚀 Compile v0/extensions in strict mode#29929
samouri merged 7 commits intoampproject:masterfrom
samouri:launchstrict

Conversation

@samouri
Copy link
Copy Markdown
Member

@samouri samouri commented Aug 21, 2020 •
edited
Loading

Closes #19380

cc @lannka

@samouri samouri self-assigned this Aug 21, 2020
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Aug 21, 2020

Hey @rsimha! These files were changed:

build-system/compile/compile.js

Hey @danielrozenberg! These files were changed:

build-system/global-configs/experiments-config.json

@google-cla google-cla bot added the cla: yes label Aug 21, 2020
rsimha
rsimha approved these changes Aug 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

One more successful experiment? Yay!

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Aug 21, 2020 •
edited
Loading

One more successful experiment? Yay!

Also fixed a couple of bugs along the way / identified one more. They were all pre-existing bugs that this experiment just highlighted.

Fixed:

Identified (lower pri, caused by pub error):

rsimha
rsimha reviewed Aug 24, 2020
View reviewed changes
@samouri
Copy link
Copy Markdown
Member Author

samouri commented Aug 25, 2020 •
edited
Loading

This is currently failing the sourcemaps check, I believe I just need to update the check to look for this new first line (most likely 'use strict', TBD

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Aug 25, 2020 •
edited
Loading

@rsimha: can you review my sourcemap-check fix?

I couldn't easily figure out where the "generated" output for a file lives. I'm guessing the fix is correct, since the start column happened to move exactly the same number of chars as in 'use strict';, but I wasn't able to trace it.

rsimha
rsimha reviewed Aug 25, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the sourcemap stuff, @samouri. I'll admit the test is not the easiest to maintain, but it's likely our best line of defense against real breakages. I've added some comments below. Let me know what you think.

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Aug 25, 2020

Thanks for the review @rsimha. You made my job really easy :)

@samouri samouri merged commit cea6c61 into ampproject:master Aug 26, 2020
@samouri samouri deleted the launchstrict branch August 26, 2020 15:58
@samouri samouri mentioned this pull request Aug 28, 2020
Closed
This was referenced Sep 15, 2020
Merged
Closed
@powerivq powerivq mentioned this pull request Sep 15, 2020
Closed
samouri added a commit that referenced this pull request Sep 15, 2020
@samouri
Revert "🚀 Compile v0/extensions in strict mode (#29929)"
2a66b7a 
This reverts commit cea6c61.
@samouri samouri mentioned this pull request Sep 15, 2020
Closed
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
@samouri @ed-bird
🚀 Compile v0/extensions in strict mode (ampproject#29929)
f507964 
* 🚀 Compile v0/extensions in strict mode

* Update compile.js

* sourcemap check: expect the usestrict

* fix fix

* raghu feedback

* lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jridgewell jridgewell jridgewell approved these changes

@dreamofabear dreamofabear dreamofabear approved these changes

+1 more reviewer

@rsimha rsimha rsimha approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@samouri samouri

Labels

cla: yes PR use: In Beta / Experimental WG: runtime

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Run v0.js in strict mode

5 participants

@samouri @jridgewell @dreamofabear @rsimha @ampprojectbot