Skip to content

update babel-plugin-transform-amp-asserts with devAssert removal.#23377

Merged
jridgewell merged 18 commits intoampproject:masterfrom
erwinmombay:multi-pipeline-extra
Aug 14, 2019
Merged

update babel-plugin-transform-amp-asserts with devAssert removal.#23377
jridgewell merged 18 commits intoampproject:masterfrom
erwinmombay:multi-pipeline-extra

Conversation

@erwinmombay
Copy link
Copy Markdown
Member

@erwinmombay erwinmombay commented Jul 17, 2019 •
edited
Loading

Currently the transformer does not remove devAssert (we should leave userAssert's alone).

@erwinmombay erwinmombay changed the title Multi pipeline extra update babel-plugin-transform-amp-asserts with devAssert removal. Jul 17, 2019
@erwinmombay erwinmombay requested review from jridgewell and rsimha July 17, 2019 21:50
@erwinmombay erwinmombay force-pushed the multi-pipeline-extra branch from b59c971 to e4910be Compare July 17, 2019 21:51
@erwinmombay
Copy link
Copy Markdown
Member Author

erwinmombay commented Jul 17, 2019

@jridgewell @rsimha PTAL

rsimha
rsimha reviewed Jul 17, 2019
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.

LGTM. Will defer to @jridgewell to review and approve.

@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Jul 19, 2019

From the single pass failure logs, I see two issues:

  • Closure is complaining while DCEing the remnants of the assert removals
  • It appears that paths are being needlessly duplicated. For example, ads/google/a4a/ads/google/a4a/utils.js

@rsimha rsimha mentioned this pull request Jul 19, 2019
Merged
@erwinmombay
Copy link
Copy Markdown
Member Author

erwinmombay commented Jul 19, 2019

@rsimha could you explain what you mean by "It appears that paths are being needlessly duplicated. For example, ads/google/a4a/ads/google/a4a/utils.js" ?

@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Jul 19, 2019 •
edited
Loading

@rsimha could you explain what you mean by "It appears that paths are being needlessly duplicated. For example, ads/google/a4a/ads/google/a4a/utils.js" ?

The path should be ads/google/a4a/utils.js (the ads/google/a4a prefix is duplicated). I'm seeing the same pattern in most of the warnings printed by closure. Perhaps there's a missing base: <PATH> option to a gulp.src() somewhere?

@mrjoro
Copy link
Copy Markdown
Member

mrjoro commented Aug 8, 2019

Is this PR still active?

@erwinmombay erwinmombay force-pushed the multi-pipeline-extra branch from 512f12c to f2c9fce Compare August 12, 2019 17:43
@jridgewell jridgewell merged commit aa7c50f into ampproject:master Aug 14, 2019
@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Aug 14, 2019

@jridgewell @erwinmombay Today, AmpPass.java eliminates all these calls:

amphtml/build-system/runner/src/org/ampproject/AmpCommandLineRunner.java

Lines 43 to 56 in d7f3349

/**
* List of string suffixes to eliminate from the AST.
*/
ImmutableSet<String> suffixTypes = ImmutableSet.of(
"dev$$module$src$log().assert()",
"dev$$module$src$log().fine()",
"dev$$module$src$log().assertElement()",
"dev$$module$src$log().assertString()",
"dev$$module$src$log().assertNumber()",
"dev$$module$src$log().assertArray()",
"dev$$module$src$log().assertBoolean()",
"devAssert$$module$src$log()",
"user$$module$src$log().fine()"
);

Does this PR remove all of them? If not, are there plans to do so?

@jridgewell
Copy link
Copy Markdown
Contributor

jridgewell commented Aug 14, 2019

They're all eliminated.

@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Aug 14, 2019 •
edited
Loading

They're all eliminated.

When I tried removing AmpPass.java, the number of instances of assert in v0.js jumped from 20 to ~80. I don't think assertElement, etc. are being removed.

@jridgewell
Copy link
Copy Markdown
Contributor

jridgewell commented Aug 14, 2019

Open an issue, please.

@rsimha rsimha mentioned this pull request Aug 14, 2019
Closed
@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Aug 14, 2019

Open an issue, please.

#23960

@rsimha rsimha mentioned this pull request Aug 19, 2019
Merged
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

+1 more reviewer

@rsimha rsimha rsimha left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@erwinmombay @rsimha @mrjoro @jridgewell @googlebot