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.

Sign up for GitHub

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

Jump to bottom

fix: exclude external imports #1193

Merged
merged 5 commits into from Aug 2, 2022
Merged

fix: exclude external imports #1193

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bd092ac
fix: exclude external imports
thepassle May 24, 2022
86d248d
test: add test
thepassle May 24, 2022
4d0c031
chore: change to http
thepassle May 28, 2022
2abb8ce
chore: feedback comments
thepassle Jun 11, 2022
6018485
refactor: review comments
thepassle Jul 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/dynamic-import-vars/src/dynamic-import-to-glob.js
View file
Open in desktop
Expand Up @@ -63,7 +63,7 @@ function expressionToGlob(node) {

export function dynamicImportToGlob(node, sourceString) {
let glob = expressionToGlob(node);
if (!glob.includes('*') || glob.startsWith('data:')) {
if (!glob.includes('*') || glob.startsWith('data:') || glob.startsWith('http')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if someone does something like import('http_helper')?

I ask this question because I believe you need to test for protocols, not for strings (even though protocols are defined by strings, but it's a stricter check is my point). You'll want a method that tests a single string for a list of protocols. e.g.

const isUrl = (what: string) => {
  try { new URL(what); return true; } 
  catch (_) { return false; }
};

const isProtocolImport = (what: string) => isUrl(what) && what.test(/^(data|http)\:(.+)/);

I dunno if that works because I suck at regex and always have to run it to verify, but you get the idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the check here 🙂 import("http_utils") should pass because there wont be a * in the glob, but I agree its good to improve the check to check for the protocol instead. I've updated the code and added another test. Could you take another look?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good callout, @shellscape. I do think this can be merged as-is, and I don’t want to delay quality work. I also have a suggestion.

Thank you, @thepassle. I would benefit from this change. I also appreciate that you added a test for http_utils.

I am wondering if this would be easier to maintain? Where;

  1. The shouldIgnore method includes the asterisk check.
  2. The URL defaults to a non-ignored file protocol, rather than a try/catch.
  3. The maintainers do not need to maintain any RegExp.

@thepassle, the changes would look something like this. Thoughts?

  if (shouldIgnore(glob)) {
    return null;
  }
const defaultProtocol = 'file:';

const ignoredProtocols = [ 'data:', 'http:', 'https:' ];
function shouldIgnore(glob) {
  const containsAsterisk = glob.includes('*');

  const globURL = new URL(glob, defaultProtocol);

  const containsIgnoredProtocol = ignoredProtocols.some(
    ignoredProtocol => ignoredProtocol === globURL.protocol
  );

  return containsAsterisk || containsIgnoredProtocol
}

If you’re thumbs up on these changes, @thepassle, @shellscape, will you please forgive me for introducing any further delays? And would there be anything I could do to prevent further delaying a merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @jonathantneal ! I've refactored the code to follow your suggestion. It required one tiny change;

- return containsAsterisk || containsIgnoredProtocol
+ return !containsAsterisk || containsIgnoredProtocol

but otherwise fine 🙂

return null;
}
glob = glob.replace(/\*\*/g, '*');
Expand Down
9 changes: 9 additions & 0 deletions packages/dynamic-import-vars/test/src/dynamic-import-to-glob.test.js
View file
Open in desktop
Expand Up @@ -17,6 +17,15 @@ test('template literal with variable filename', (t) => {
t.is(glob, './foo/*.js');
});

test('external', (t) => {
const ast = CustomParser.parse('import(`https://some.cdn.com/package/${version}/index.js`);', {
sourceType: 'module'
});

const glob = dynamicImportToGlob(ast.body[0].expression.arguments[0]);
t.is(glob, null);
});

test('data uri', (t) => {
const ast = CustomParser.parse('import(`data:${bar}`);', {
sourceType: 'module'
Expand Down