I2I: Consolidate source code under `src/`

[rcebulko]

In the earlier days of amp, the root directory was fairly sparse. "Top-level" directories like 3p, ads, builtins, and extensions had separate functions, while src was synonymous with the AMP runtime. IIUC, one motivation for this at the time was so external contributors could easily view the repo and understand where to contribute an ad provider, 3p integration, or new component/extension.

Today, the root directory has dozens of config files and directories (.babel-cache/, .circleci/, .codecov.yml, .css-cache/, .editorconfig, .eslintignore, .eslintplugin.js, .eslintrc.js, .git/, .gitattributes, .github/, .gitignore, .lando.yml, .lgtm.yml, .npmrc, .nyc_output/, .pre-closure-cache/, .prettierignore, .prettierrc, .renovaterc.json, .vscode/), build/dist files, node_modules, third_party, and a handful of other directories containing tools, docs, and more.

More generally, I suspect if we re-created the repo today, we'd put all the source files under src, separate from built system, build files, documentation, and testing. Down the line, this can also allow for consolidation and simplification of some lint rules, presubmits, etc.

[samouri]

In general I'm in favor of moving all of our code that gets bundled/distributed to src. Note though that some of the examples above (i.e. .babel-cache and .css-cache) aren't actually part of our git repo.

[erwinmombay]

+1 on this. this would help infra code quite a bit i think. generally when we add a new directory we would need to add a new glob to some rule or transformer which always seemed very brittle. with this change there would most likely a reduction in the globs for linters and compilers which should reduce a little bit of complexity

[caroqliu]

One concern is that this will make it more difficult to view the blame of the moved directories, whose original paths will be effectively treated as deleted. It's still possible by calling git blame with a commit hash prior to the relocation, but retrieving this is slightly more friction than not and could be hard for new contributors as time goes on. For me, being able to do this is valuable for components I'm less familiar with as well as those with long histories.

Another point -- when moving extensions inside src, would this change then move any unit, e2e, and validator tests typically located inside extensions/**/test outside of src to the separate testing directory described?

[rcebulko]

One concern is that this will make it more difficult to view the blame of the moved directories, whose original paths will be effectively treated as deleted. It's still possible by calling git blame with a commit hash prior to the relocation, but retrieving this is slightly more friction than not and could be hard for new contributors as time goes on. For me, being able to do this is valuable for components I'm less familiar with as well as those with long histories.

As long as I used git mv properly (which I intend to), history/blame still works naturally within GitHub. For example, when I moved src/utils/string.js to src/core/types/string/index.js, you can still see the blame from 6 years ago on the "new"/moved file. So, I think that may not be an issue, unless I'm missing something.

Another point -- when moving extensions inside src, would this change then move any unit, e2e, and validator tests typically located inside extensions/**/test outside of src to the separate testing directory described?

I would say moving any test files within extensions/, if anything, would merit an entirely separate design review.

Personal tangent on what I think we should do with test files down the line, in separate design review(s)

As an aside, personally I would be in favor of moving more tests closer to source files (and `test/unit/core` is structured to mirror the source tree so this could be done easily for core). If that were to happen, I'd propose in conjunction shifting the naming convention from `foo.js`/`test-foo.js` to `foo.js`/`foo.test.js` or `foo-test.js` so source+test files are grouped together in sorting. But that would again be at least a separate design review, if not two.

Regardless of the note above, the scope of this DR would be almost exclusively git mv {3p,ads,builtins,extensions} src/ (and updating corresponding imports), and not any shuffling of test files at this time.

[rcebulko]

Follow-up: For those with active open branches, merging may not play nice with the moved files, while rebasing should apply the local changes to the moved directories without issue. Since this still has the potential to cause disruption to developers, each directory should be moved separately and with (~1wk?) advanced notice, to make sure anyone actively working in those directories (especially extensions) is aware of the merge/rebase difference

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.