Skip to content

Commit

Permalink
♻️ Extract type assertions out of log to multiple pure function calls (
Browse files Browse the repository at this point in the history
…#34088)

* Make assert its own directory

* Fix type annotation on isString

* Update log

* Move type assertion logic out of log

* Provide user/dev assertion modules

* Take assertFn as parameter

* Add comments

* Support Array args to assertion functions

* Missing things

* Remove commented-out import

* Remove outdated comment

* Remove sentinel param from dev/user assert files

* DCE escape hatch on dev assert helpers

* Add typecasts for DCE bail-outs

* Update forbidden terms allowlist
  • Loading branch information
rcebulko authored May 4, 2021
1 parent 20a587e commit f2a49b8
Show file tree
Hide file tree
Showing 10 changed files with 792 additions and 243 deletions.
12 changes: 12 additions & 0 deletions build-system/test-configs/dep-check-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,18 @@ exports.rules = [
allowlist: ['src/amp.js->src/polyfills/index.js'],
},

// Base assertions should never be used explicitly; only the user/dev wrappers
// or the Log class should have access to the base implementations.
{
filesMatching: '**/*.js',
mustNotDependOn: 'src/core/assert/base.js',
allowlist: [
'src/core/assert/dev.js->src/core/assert/base.js',
'src/core/assert/user.js->src/core/assert/base.js',
'src/log.js->src/core/assert/base.js',
],
},

// Rules for main src.
{
filesMatching: 'src/**/*.js',
Expand Down
3 changes: 2 additions & 1 deletion build-system/test-configs/forbidden-terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,8 @@ const forbiddenTermsGlobal = {
'/\\*\\* @type \\{\\!Element\\} \\*/': {
message: 'Use assertElement instead of casting to !Element.',
allowlist: [
'src/log.js', // Has actual implementation of assertElement.
'src/core/assert/base.js', // Has actual implementation of assertElement.
'src/core/assert/dev.js', // Has actual implementation of assertElement.
'src/polyfills/custom-elements.js',
'ads/google/imaVideo.js', // Required until #22277 is fixed.
'3p/twitter.js', // Runs in a 3p window context, so cannot import log.js.
Expand Down
193 changes: 0 additions & 193 deletions src/core/assert.js

This file was deleted.

25 changes: 25 additions & 0 deletions src/core/assert/assert.extern.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* Copyright 2021 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* @fileoverview Type externs for assertions.
* @externs
*/

/**
* @typedef {function(?, string=, ...*):?|function(?, !Array<*>)}
*/
let AssertionFunction;
Loading

0 comments on commit f2a49b8

Please sign in to comment.