fix: exclude external imports

[thepassle]

Rollup Plugin Name: dynamic-import-vars

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Currently the plugin will fail my build when I import something like:

import(`https://some.cdn.com/package/${version}/index.js`);

Since this import is 'external', and doesn't need to look on the filesystem, these kind of imports should be allowed to pass through and go untouched by the plugin.

[LarsDenBakker]

Looks good to me!

[thepassle]

Any chance we can get this merged? 🙂 Its a small change, and it would help me a lot

[LarsDenBakker]

@shellscape I have merge rights but I don't work on the repo that often, and I think I also can't publish new versions. would be great if you have a moment to do it

[shellscape]

Will have a look today. Thanks for the ping

[thepassle]

Will have a look today. Thanks for the ping

Friendly reminder 🙂

[shellscape]

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.

[thepassle]

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?

[jonathantneal]

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?

[thepassle]

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 🙂

[thepassle]

@shellscape Any chance you could take another look at this? Would love to get this merged

[thepassle]

@lukastaegert @shellscape Could this please get merged? This has already been reviewed by Jon, Lars, and Andrew at this point. It's only waiting to get merged, I dont see any other reason to block this from moving forward.

[shellscape]

Sure :)

[thepassle]

Thank you!