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

Allow method calls of import (fixes #135) #150

Merged
merged 1 commit into from
Oct 1, 2020

Conversation

gijsk
Copy link
Contributor

@gijsk gijsk commented Sep 30, 2020

As noted in #135 (comment) , in modern parsers this should be OK as the "real" dynamic import cannot be renamed or called as a method.

@mozfreddyb
Copy link
Collaborator

liberal parser support can bite us here.
I've found various way this is parsed, by going through eslint, babel-eslint, @babel/parser on https://astexplorer.net/:

  • acorn, babel-eslint ImportExpression
  • @babel/parser: CallExpression with "type": "Import"
  • flow (which we dont care about): CallExpression with a callee that is not an Identifier node, but rather a node of type Import

We don't care about flow, but can we make sure that we catch the others with tests per-parser?

@gijsk gijsk force-pushed the import-method branch 3 times, most recently from c1bdc05 to ae92378 Compare October 1, 2020 11:35
@mozfreddyb mozfreddyb self-requested a review October 1, 2020 11:41
@mozfreddyb mozfreddyb merged commit 55c595b into mozilla:master Oct 1, 2020
@mozfreddyb
Copy link
Collaborator

Bedankt! :)

Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this pull request Oct 4, 2020
…t/jasmine-boot.js`

With the latest release of the `eslint-plugin-no-unsanitized` package, we no longer need to disable this rule; see mozilla/eslint-plugin-no-unsanitized#150
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this pull request Oct 4, 2020
…t/jasmine-boot.js`

With the latest release of the `eslint-plugin-no-unsanitized` package, we no longer need to disable this rule; see mozilla/eslint-plugin-no-unsanitized#150
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

Successfully merging this pull request may close these issues.

2 participants