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.

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

runtime: convert services to standard object #36486

Merged
merged 3 commits into from Oct 26, 2021

Conversation

samouri
Copy link
Member

@samouri samouri commented Oct 25, 2021

summary

More palatable alternative to #36400. Addresses part of #35264 (comment), and #36453
Converts Services to a regular object instead of a class with static methods.
This allows terser to DCE unused Services, whereas Closure Compiler seems to lose the ability.

before

class Services {
  ampdocFor() {...}
  bindForDocOrNull() {...}
}

after

const Services =  {
  ampdocFor: () => {...},
  bindForDocOrNull: () => {...}
}

results

v0.mjs brotli v0.mjs
closure compiler on main 60.87 212.31
closure compiler on this branch 60.91 212.76
esbuild on this branch 61.76 223.99
esbuild on main 62.50 229.95

@samouri
Copy link
Member Author

samouri commented Oct 26, 2021

First issue found: terser will not dce an object if that object has any self-references (which Services does)

@samouri
Copy link
Member Author

samouri commented Oct 26, 2021

Second issue, it also doesn't like shorthand syntax for when names match
EDIT opened up a terser FR to address both issues: terser/terser#1097.

For now we have workarounds (no self-references or obj shorthand)

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.

None yet

3 participants