-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
I2I: Consolidate source code under src/
#34866
Comments
In general I'm in favor of moving all of our code that gets bundled/distributed to |
+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 |
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 Another point -- when moving |
As long as I used
I would say moving any test files within 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 |
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 |
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. |
In the earlier days of amp, the root directory was fairly sparse. "Top-level" directories like
3p
,ads
,builtins
, andextensions
had separate functions, whilesrc
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.The text was updated successfully, but these errors were encountered: