♻️ Guard dependencies in `src/{context,core,preact}` by rcebulko · Pull Request #33016 · ampproject/amphtml

rcebulko · 2021-03-02T21:19:01Z

This PR:

adds the import/no-restricted-paths eslint rule
- prohibits src/core from importing outside of itself
- prohibits src/preact from importing outside of itself, src/core, or src/context
- prohibits src/context from importing outside of itself or src/core
moves several preact-only helper files (src/loading, src/contextprops) into src/preact
update all assertion imports in src/preact and src/context to use new helpers

Remaining excluded files generate linter warnings:

amp-owners-bot · 2021-03-02T21:19:08Z

Hey @jridgewell! These files were changed:

src/core/.eslintrc.js
src/core/contextprops.js
src/core/loading-instructions.js

rcebulko · 2021-03-02T21:25:18Z

src/.eslintrc.js

+            // Disallow importing AMP dependencies into core
+            'target': 'src/core',
+            'from': 'src',
+            'except': ['./core'],


Paths for target and from are absolute; paths in except are relative to the from path. Weird, I know.

rcebulko · 2021-03-02T21:26:55Z

src/core/.eslintrc.js

-      'error',
-      {
-        // Disallow importing dependencies from outside this directory
-        'patterns': ['..'],


This did not work as desired with subfolders

rcebulko · 2021-03-02T21:27:14Z

src/.eslintrc.js

+        './context/scheduler.js',
+        './context/values.js',
+      ],
+      'rules': {'import/no-restricted-paths': 1},


Plan: Warning for now; once exclusions are migrated, everything errors.

rcebulko · 2021-03-02T21:28:32Z

src/preact/context.js


 import * as Preact from './index';
-import {Loading, reducer as loadingReducer} from '../loading';
+import {Loading, reducer as loadingReducer} from './loading-instructions';


This removes the only restricted dependency, so this file was removed from the allowlist

rcebulko · 2021-03-02T21:28:54Z

src/preact/contextprops.js

 */

-import {Loading, reducer as loadingReducer} from '../loading';
+import {Loading, reducer as loadingReducer} from './loading-instructions';


This removes the only restricted dependency, so this file was removed from the allowlist

Can we instead whitelist src/context and src/contextprops? Or move them to src/pure? They have been intentionally constructed as "pure". There might be some dependencies there now that have crept in, but I do think we can and should be "pure".

rcebulko · 2021-03-02T21:29:44Z

src/preact/slot.js

-import {CanPlay, CanRender, LoadingProp} from '../contextprops';
-import {dev} from '../log';
+import {CanPlay, CanRender, LoadingProp} from './contextprops';
+import {devAssert} from '../core/assert';


These removes the only restricted dependencies, so this file was removed from the allowlist.

jridgewell · 2021-03-02T22:24:03Z

src/preact/contextprops.js

@@ -14,7 +14,7 @@
 * limitations under the License.
 */

-import {Loading, reducer as loadingReducer} from '../loading';


I think these actually belong in src/core? They're meant to be used by both Bento and regular Components?
/cc @dvoytenko

AFAICT is file is not imported anywhere else in the repo except the files in this PR

dvoytenko · 2021-03-03T21:28:42Z

src/preact/contextprops.js

 */

-import {Loading, reducer as loadingReducer} from '../loading';
+import {Loading, reducer as loadingReducer} from './loading-instructions';


Can we instead whitelist src/context and src/contextprops? Or move them to src/pure? They have been intentionally constructed as "pure". There might be some dependencies there now that have crept in, but I do think we can and should be "pure".

src/preact/slot.js

jridgewell · 2021-03-03T23:31:18Z

build-system/tasks/lint.js

      );
    }
-    process.exitCode = 1;
+    process.exitCode = errorCount || !isCiBuild() ? 1 : 0;


Can you explain this change?

Errors should always fail the build
Warnings (such as for the allowlisted imports) should not fail the build on CI
Chatted offline with @rsimha , looping in for approval on this file

I didn't mean for this change to be made to lint.js. We want to continue failing CI builds when there are warnings.

Instead, you can change line 63 in src/.eslintrc.js as follows:

'rules': {'import/no-restricted-paths': isCiBuild() ? 0 : 1},

This will let you see warnings within src when you're doing local development, but not let those warnings show up during CI. Meanwhile, PRs that introduce unaccounted for warnings in other directories will be flagged during CI.

That makes sense; will do

Necromancing this conversation thread for a moment. With this configuration, branches touching any files with warnings get picked up as build targets, and lint emits warnings which in turn fails the task with a non-zero exit code. This means during local development, amp pr-check gets held up by the warnings and can't be useful on these branches. @rsimha do you have any suggestions to work around this? I'd like warnings to be displayed without failing the whole set of checks if possible. Could warnings not fail lint on non-CI builds? I don't see any lint rules that are warnings currently

src/preact/slot.js

Addressed comments; approval from Justin

rcebulko requested a review from jridgewell March 2, 2021 21:19

rcebulko commented Mar 2, 2021

View reviewed changes

jridgewell reviewed Mar 2, 2021

View reviewed changes

rcebulko added 11 commits March 3, 2021 12:35

Add eslint comments

a75408e

Support subfolders via no-restricted-paths plugin

688449b

Move src/loading to src/core/loading-instructions

01cb888

Remove exception for src/preact/context.js

fea9629

Update assertions to use src/core/assert

3987f3a

Move src/contextprops into src/preact

7c7de00

Restrict imports in src/context and allow imports from it

28bb0db

Remove exception for src/preact/slot.js

810abb6

Move loading-instructions into src/preact

6d3ce58

Fix imports

fd27b51

Fix dev assertion

2a8824d

rcebulko force-pushed the core-preact branch from 17c0db3 to 2a8824d Compare March 3, 2021 17:35

Allow warnings in ci builds

f161115

rcebulko force-pushed the core-preact branch from 40e9d7b to f161115 Compare March 3, 2021 21:07

dvoytenko previously requested changes Mar 3, 2021

View reviewed changes

Move loading-instructions into src/core

15d4ef4

rcebulko requested a review from dvoytenko March 3, 2021 21:33

Move contextprops into src/core

de3c127

rcebulko requested a review from jridgewell March 3, 2021 21:43

Allow import in contextprops

6e9afd8

jridgewell reviewed Mar 3, 2021

View reviewed changes

Review comments

4835760

rcebulko force-pushed the core-preact branch from 65bef60 to 4835760 Compare March 4, 2021 01:23

rcebulko requested a review from jridgewell March 4, 2021 03:44

jridgewell approved these changes Mar 4, 2021

View reviewed changes

rcebulko merged commit fe5417d into ampproject:master Mar 4, 2021

rcebulko deleted the core-preact branch March 4, 2021 19:18

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Mar 10, 2021

caroqliu mentioned this pull request Apr 5, 2021

Tracking Issue: restricted paths eslint allowlist #33631

Closed

Conversation

rcebulko commented Mar 2, 2021

Uh oh!

amp-owners-bot bot commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsimha Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amp-owners-bot bot commented Mar 2, 2021 •

edited

Loading

rsimha Mar 4, 2021 •

edited

Loading