Skip to content

core mode: add version()#35298

Merged
samouri merged 2 commits intoampproject:mainfrom
samouri:mode-internal-runtime-version
Aug 4, 2021
Merged

core mode: add version()#35298
samouri merged 2 commits intoampproject:mainfrom
samouri:mode-internal-runtime-version

Conversation

@samouri
Copy link
Copy Markdown
Member

@samouri samouri commented Jul 19, 2021 •
edited
Loading

summary

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

@samouri samouri force-pushed the mode-internal-runtime-version branch 2 times, most recently from 86542de to 9353008 Compare July 19, 2021 21:02
@samouri samouri self-assigned this Jul 19, 2021
@samouri samouri force-pushed the mode-internal-runtime-version branch from 99f688c to f53e7f6 Compare July 20, 2021 15:20
@samouri samouri requested a review from rcebulko July 20, 2021 15:23
@samouri samouri force-pushed the mode-internal-runtime-version branch from f53e7f6 to 888f342 Compare July 20, 2021 15:25
@samouri samouri requested a review from jridgewell July 20, 2021 16:17
@samouri samouri marked this pull request as ready for review July 20, 2021 16:17
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Jul 20, 2021 •
edited
Loading

Hey @jridgewell! These files were changed:

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

rcebulko
rcebulko approved these changes Jul 20, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Member Author

samouri commented Jul 20, 2021 •
edited
Loading

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
Copy link
Copy Markdown
Contributor

rcebulko commented Jul 20, 2021

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
Copy link
Copy Markdown
Member Author

samouri commented Jul 21, 2021 •
edited
Loading

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
Copy link
Copy Markdown
Contributor

rcebulko commented Jul 21, 2021

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 👍🏻

@samouri samouri force-pushed the mode-internal-runtime-version branch from 888f342 to 9e7c915 Compare July 22, 2021 16:50
rcebulko
rcebulko reviewed Jul 22, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Member Author

samouri commented Jul 22, 2021 •
edited
Loading

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
Copy link
Copy Markdown
Contributor

rcebulko commented Jul 22, 2021

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
Copy link
Copy Markdown
Member Author

samouri commented Jul 30, 2021 •
edited
Loading

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

@samouri samouri force-pushed the mode-internal-runtime-version branch 3 times, most recently from b42cbf6 to 276aee1 Compare July 30, 2021 18:21
@samouri samouri mentioned this pull request Jul 30, 2021
Merged
rcebulko
rcebulko approved these changes Jul 30, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

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 😆

@samouri samouri force-pushed the mode-internal-runtime-version branch from 276aee1 to d334977 Compare August 3, 2021 20:25
@samouri samouri force-pushed the mode-internal-runtime-version branch from d334977 to 40eb85e Compare August 3, 2021 21:42
@samouri samouri changed the title core mode: add internalRuntimeVersion core mode: add version() Aug 3, 2021
@samouri samouri merged commit 85ef341 into ampproject:main Aug 4, 2021
@Mstafamhamadmhamad

This comment has been minimized.

Sign in to view
@samouri samouri deleted the mode-internal-runtime-version branch August 9, 2021 14:18
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

@rcebulko rcebulko rcebulko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@samouri samouri

Labels

PR use: In Beta / Experimental PR use: In LTS PR use: In Stable Type: Refactoring WG: performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@samouri @rcebulko @Mstafamhamadmhamad @jridgewell @ampprojectbot