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 the skip option #396

Merged
merged 2 commits into from
Nov 12, 2023
Merged

Fix the skip option #396

merged 2 commits into from
Nov 12, 2023

Conversation

koddsson
Copy link
Collaborator

@koddsson koddsson commented Nov 6, 2023

Fixes #384.

This change starts accepting a regular expression proper as a skip option. This seems to have been indicated by the existing types and counter-intuitively the types that are actually accepted aren't represented in the TypeScript declaration.

This pull request adds the string[] | string types to the skip property in the options type and creates a skip wrapper in case the variable passed in is a regular expression.

@koddsson
Copy link
Collaborator Author

koddsson commented Nov 6, 2023

Damn the firefox tests seem flaky in CI 😭. Here's the failing build for posterity while I re-run the build: https://github.com/guybedford/es-module-shims/actions/runs/6772254424/job/18404391999?pr=396

@koddsson
Copy link
Collaborator Author

koddsson commented Nov 6, 2023

Thinking about this more I feel like I should add tests here since this wasn't caught by tests already.

@guybedford
Copy link
Owner

This is great thank you!

Yes, tests would be preferable.

The Firefox test flakes are a really weird thing where the Firefox binary itself gets cleaned up and no longer exists! Haven't figured that one out to date...

@guybedford
Copy link
Owner

Note - I'm waiting on landing this in case you want to add tests.

@koddsson
Copy link
Collaborator Author

Note - I'm waiting on landing this in case you want to add tests.

I do want to add tests but I couldn't figure out the test setup.

Am I on the right tracking thinking I should add something like:

<script type="module-shim">
  import test from "skipped.js";
  order.push(6);
</script>

and a "skip": new RegExp(/skipped.js/) to the options bag in test/test-shim.html. And then I can add a assertion in test/shim.js that the library has delegated the load of skipped.js to the native module loader? (I haven't figured out how to do that assertion yet).

@guybedford
Copy link
Owner

Yeah agreed this is hard to test. I mean, it clearly works, so we can just land it too if it's too tricky to figure out a loader check.

@guybedford guybedford merged commit 27c5226 into guybedford:main Nov 12, 2023
7 checks passed
@koddsson koddsson deleted the fix-skip-option branch November 12, 2023 09:05
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.

mismatch between code and type
2 participants