Skip to content
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

Fix all of our tests to be ESM compatible. #34317

Closed
samouri opened this issue May 11, 2021 · 4 comments
Closed

Fix all of our tests to be ESM compatible. #34317

samouri opened this issue May 11, 2021 · 4 comments
Labels
Stale Inactive for one year or more Type: Bug

Comments

@samouri
Copy link
Member

samouri commented May 11, 2021

summary
As discovered by @jridgewell and esbuild, our tests are all using import/export and yet some of them still do things that are technically illegal. They modify the namespaced import. For the full list see #33443 (comment).

Once all incorrect usages of esm are fixed, we can turn off module transpilation from our pre-esbuild babel transformation for out tests.

cc @jridgewell / @rsimha.

PS: @jridgewell: can you share the rule that let you discover the list of breakages?

@jridgewell
Copy link
Contributor

// build-system/eslint-rules/no-ambiguous-namespace-import.js

/**
 * Copyright 2018 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.
 */
'use strict';

module.exports = function (context) {
  const namespaces = new Set();
  return {
    Program(node) {
      const imports = node.body.filter((n) => n.type === 'ImportDeclaration');
      imports.forEach((node) => {
        const specifiers = node.specifiers.filter(
          (n) => n.type === 'ImportNamespaceSpecifier'
        );
        specifiers.forEach((s) => namespaces.add(s.local.name));
      });
    },

    'Program:exit': function () {
      namespaces.clear();
    },

    Identifier(node) {
      switch (node.parent.type) {
        case 'ImportNamespaceSpecifier':
          return;
        case 'MemberExpression':
          if (node.parent.object === node) {
            return;
          }
      }
      if (!namespaces.has(node.name)) {
        return;
      }
      context.report({
        node,
        message: `Ambiguous use of a namespace import. All uses must be \`${node.name}.foo\` property accesses`,
      });
    },
  };
};

This is a little too overzealous, but would prevent every bad use.

@jridgewell
Copy link
Contributor

There's also a slightly less zealous https://github.com/benmosher/eslint-plugin-import/blob/HEAD/docs/rules/namespace.md, but that doesn't catch sandbox.stub(ns, 'export') indirections.

@samouri
Copy link
Member Author

samouri commented Aug 18, 2021

cc @alanorozco who was interested in using sinon to stub the Services namespace object.

@stale
Copy link

stale bot commented Oct 1, 2022

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.

@stale stale bot added the Stale Inactive for one year or more label Oct 1, 2022
@samouri samouri closed this as completed Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Inactive for one year or more Type: Bug
Projects
None yet
Development

No branches or pull requests

2 participants