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

Duplicate import aliases bug #87

Open
T9-Forever opened this issue Apr 7, 2024 · 3 comments
Open

Duplicate import aliases bug #87

T9-Forever opened this issue Apr 7, 2024 · 3 comments

Comments

@T9-Forever
Copy link

import { t as $t, t } from '@lingui/macro'

t`Hello`

$t`Hello`

Only the last t variable will be registered in imports_id_map

Maybe need to add a secondary data structure importsBindingMap?

// export name -> local name
importsIdMap: Map<string, string>
// local name -> export name
importsIdMapInverted: Map<string, string>


// local name -> ast node
importsBindingMap: Map<string, Set<Identifier>>

If swc has babel's scope.getBinding, it can maintain the importsBindingMap during the registration phase.

Please extend swc's capabilities~

@T9-Forever
Copy link
Author

T9-Forever commented Apr 7, 2024

I've rewritten the babel plugin before, because of the risk of discrepancies between the official swc and babel translations.
(Native babel plugin for extract & swc plugin for runtime)
It may be that the swc plugin can also go through the following

  public safeAddImportsBinding(name: string, node: Identifier) {
    const st = this.importsBindingMap.get(name) ?? new Set<Identifier>()
    // safeUpdate
    this.importsBindingMap.set(name, st)
    st.add(node)
  }

  public safeHasImportsBinding(name: string, node: Identifier): boolean {
    const st = this.importsBindingMap.get(name) ?? new Set<Identifier>()
    // safeUpdate
    this.importsBindingMap.set(name, st)
    return st.has(node)
  }

  public registerMacroImport(impPath: NodePath<ImportDeclaration>) {
    impPath.node.specifiers.forEach((spec) => {
      if (t.isImportSpecifier(spec)) {
        const imported = spec.imported
        const local = spec.local
        if (t.isIdentifier(imported)) {
          this.importsIdMap.set(imported.name, local.name)
          this.importsIdMapInverted.set(local.name, imported.name)
          const bindng = impPath.scope.getBinding(local.name)
          bindng?.referencePaths.forEach((path) => {
            this.safeAddImportsBinding(local.name, path.node as Identifier)
          })
        }
      }
    })
  }
  
  public isLinguiIdent(name: string, ident: Identifier): boolean {
    return (
      this.importsIdMap.get(name) === ident.name &&
      this.safeHasImportsBinding(ident.name, ident)
    )
}

@timofei-iatsenko
Copy link
Collaborator

babel's version uses native babel's capabilities already and doesn't have this flaw. SWC version, indeed, should be improved for that case.

PS

What is the real usecase of this snippet?

 import { t as $t, t } from '@lingui/macro'

@T9-Forever
Copy link
Author

T9-Forever commented Apr 9, 2024

babel's version uses native babel's capabilities already and doesn't have this flaw. SWC version, indeed, should be improved for that case.

PS

What is the real usecase of this snippet?

 import { t as $t, t } from '@lingui/macro'

It's a long story. At first our code only supported English, then I used jscodeshift to convert plain text to lingui syntax.

const renderString = 'Hello';
const renderEle = <div>Hello</div>;

auto translate by script

import {t as $t, Trans} from '@lingui/macro';
const renderString = $t`Hello`;
const renderEle = <div><Trans>Hello</Trans></div>;

With the previous strategy, $t looks safer (since I don't know the history of the code, t may have conflicts).
Of course it might look a bit silly to write the $t yourself, if it's not transformed by scripts.

Perhaps because of the large number of alias t in the project, teammates have gotten used to writing them. (Stockholm Syndrome, perhaps...)

However, if there are developers at different levels in the project, there is a risk of an production incident if swc does not convert multiple aliases.

It's hard to find problems in compile detection unless you add eslint rules manually (which I'm currently planning to do). However, as an underlying framework, it is necessary for swc to support basic js syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants