core mode: add version()

[samouri]

summary

Adds .version to #core/mode. Removes it from src/internal-version.js.

[amp-owners-bot]

Hey @jridgewell! These files were changed:

src/core/mode/index.js
src/core/mode/version.js

[rcebulko]

I know this used to live in mode, but WDYT about moving it into #core/constants? Then again, I suppose the same could be said of all the mode constants, so I'm not sure what makes the most sense. Approving to unblock.

[samouri]

I know this used to live in mode, but WDYT about moving it into #core/constants? Then again, I suppose the same could be said of all the mode constants, so I'm not sure what makes the most sense. Approving to unblock.

What is mode but a set of constants :)

[rcebulko]

I know this used to live in mode, but WDYT about moving it into #core/constants? Then again, I suppose the same could be said of all the mode constants, so I'm not sure what makes the most sense. Approving to unblock.

What is mode but a set of constants :)

Things like minified/esm/fortesting etc are really the "mode" in which it was compiled, whereas the RTV is very much a constant. My feeling is that getMode got overloaded because it became the place to dump compile-time-ish constants, but really I think this could/should just be a const INTERNAL_RTV = "..."; in #core/constants/internal-rtv, and bring mode back to just representing the binary/execution mode

[samouri]

I know this used to live in mode, but WDYT about moving it into #core/constants? Then again, I suppose the same could be said of all the mode constants, so I'm not sure what makes the most sense. Approving to unblock.

What is mode but a set of constants :)

Things like minified/esm/fortesting etc are really the "mode" in which it was compiled, whereas the RTV is very much a constant. My feeling is that getMode got overloaded because it became the place to dump compile-time-ish constants, but really I think this could/should just be a const INTERNAL_RTV = "..."; in #core/constants/internal-rtv, and bring mode back to just representing the binary/execution mode

I'd push back on the concept of "mode it is compiled in". I don't know of a good definition for what constitutes a mode and what doesn't. Instead I'd argue the clean separation of constants exists between build constants vs. source constants.

[rcebulko]

I'd push back on the concept of "mode it is compiled in". I don't know of a good definition for what constitutes a mode and what doesn't. Instead I'd argue the clean separation of constants exists between build constants vs. source constants.

I don't feel strongly enough, this is fine by me 👍🏻

[rcebulko]

I'm curious if it would be sensible to rename this to just mode.rtv()? I feel like it's a ubiquitous-enough term that it would be very easily understood

[samouri]

I'm curious if it would be sensible to rename this to just mode.rtv()? I feel like it's a ubiquitous-enough term that it would be very easily understood

I'm fine with that.

Separate note: DCE doesn't seem to be working. I think we can make the amp-mode transform also operate on the namespace import of import * as mode from #core/mode

[rcebulko]

I'm curious if it would be sensible to rename this to just mode.rtv()? I feel like it's a ubiquitous-enough term that it would be very easily understood

I'm fine with that.

Separate note: DCE doesn't seem to be working. I think we can make the amp-mode transform also operate on the namespace import of import * as mode from #core/mode

Working for what? DCE should be working for the true/false values; I don't believe it would inline the RTV string if that's what you're expecting.

[samouri]

I'm curious if it would be sensible to rename this to just mode.rtv()? I feel like it's a ubiquitous-enough term that it would be very easily understood

I'm fine with that.

Separate note: DCE doesn't seem to be working. I think we can make the amp-mode transform also operate on the namespace import of import * as mode from #core/mode

Actually...this isn't rtv(), it is version()...updated accordingly.
Also will update the babel plugin (but split it out to a new PR once it works) so that we can land this without a size regression

[rcebulko]

This is great! Thanks for doing it
Side note: I've found it easier to shift logic to core in one PR and have mode.js reference it, then (in theory) eventually update callsites/transforms in a separate PR. If you feel motivated to keep working on these, uses of getMode(win).test, and getMode(win).localDev could both be updated to the core versions as well; I moved the logic to core, but didn't update all the files because of OWNERS approval 😆