Skip to content

run v0 and extensions in strict mode#28793

Merged
samouri merged 10 commits intoampproject:masterfrom
samouri:strict
Jul 14, 2020
Merged

run v0 and extensions in strict mode#28793
samouri merged 10 commits intoampproject:masterfrom
samouri:strict

Conversation

@samouri
Copy link
Copy Markdown
Member

@samouri samouri commented Jun 10, 2020

Fixes #19380.

From mdn:

Fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that's not strict mode.

note: in some ways this is a baby step towards esm, since those force strict mode as well.

cc @choumx / @jridgewell

@amp-owners-bot amp-owners-bot bot requested a review from rsimha June 10, 2020 03:52
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Jun 10, 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

@samouri samouri removed the request for review from rsimha June 10, 2020 03:52
@samouri samouri self-assigned this Jun 10, 2020
@jridgewell jridgewell requested review from jridgewell and rsimha June 11, 2020 02:11
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.

LGTM with a couple of comments.

}
while (this.unlisteners_.length > 0) {
this.unlisteners_.pop().call();
this.unlisteners_.pop()();
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this one looks safe to me since exitHandler is bound via arrow funcs.

return;
}
this.methods_[method].call();
this.methods_[method]();
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm having trouble verifying if this change is safe, or if we should be setting this to a value. I'll investigate before merging

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Jul 9, 2020

Do experiments work on build-system files? This is failing lint, and it would make sense to me if these global consts were only valid within the scope of CC.

@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Jul 10, 2020

Do experiments work on build-system files? This is failing lint, and it would make sense to me if these global consts were only valid within the scope of CC.

Just remembered: you'll need to add the new constant to build-system/global-configs/experiments-const.json.

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Jul 10, 2020

Do experiments work on build-system files? This is failing lint, and it would make sense to me if these global consts were only valid within the scope of CC.

Just remembered: you'll need to add the new constant to build-system/global-configs/experiments-const.json.

I added it there! https://github.com/ampproject/amphtml/pull/28793/files#diff-67d2d24ff5678f99cda13ca5af136ebeR5

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Jul 13, 2020

Do experiments work on build-system files? This is failing lint, and it would make sense to me if these global consts were only valid within the scope of CC.

I don't think experiments work on build-system, since it ends up only affecting preclosure babel transforms (which build-system isn't run through). I've modified the code to listen for the correct --define_experiment_constant arg, which should produce a similar behavior.

I've manually tested this via:

  • gulp --compiled: no delta, no "use strict" in the code.
  • gulp --compiled --define_experiment_constant STRICT_COMPILATION: "use strict" inserted at the top of the closure.

@samouri samouri requested a review from rsimha July 13, 2020 17:14
@samouri samouri merged commit 09e87e8 into ampproject:master Jul 14, 2020
@samouri samouri deleted the strict branch July 14, 2020 20:29
mszylkowski pushed a commit to mszylkowski/amphtml that referenced this pull request Jul 22, 2020
* run v0 in strict mode

* if working before, this should work

* add rtv experiment

* raghu comments

* default val

* unbreak / lint

* create server-side experiment manually

* raghu style

* lint :/

* doih
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.

Run v0.js in strict mode

6 participants