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

Detect minified umd with leading fn declarations #788

Merged
merged 6 commits into from
Jan 16, 2024

Conversation

lewisl9029
Copy link
Contributor

@lewisl9029 lewisl9029 commented Jan 11, 2024

Hi there! This PR adds a fix for a minified UMD package (ttag) that we didn't handle properly due to the hard-coded indices on statements inside the UMD fn body for detecting webpack's .d and .r calls.

You can repo by pasting this into the esm.sh playground:

import * as ttag from "https://esm.sh/ttag";
console.log('ttag', ttag)

Note that we only see a default export even though the package has a whole bunch of named exports that we should be detecting.

This new implementation now uses some slightly smarter heuristics to determine indices in some places (skipping 1 index only if we detect "use strict"), and searches the entire list of statements until we find one that matches the expected shape in others. I added a few new test cases that fails with the previous implementation and succeeds with the current one.

I wrote this part of the code originally in #689, and tbh it still feels pretty nasty to work with... I'm still not familiar enough with idiomatic rust to know how to make things better yet. Would love to get some tips!

@lewisl9029
Copy link
Contributor Author

lewisl9029 commented Jan 15, 2024

Added another test case for https://www.npmjs.com/package/@microsoft/teams-js?activeTab=readme
And refactored out a bunch of duplicated logic, which should hopefully make this a bit less painful to work with. Let me know if you have any other suggestions for improvements!

Also limiting to first 8 statements to make sure search time is bounded for large modules, since I noticed some performance regressions when I tried the previous version of my fork: https://www.npmjs.com/package/@lewisl9029/esm-cjs-lexer

Copy link
Member

@ije ije left a comment

Choose a reason for hiding this comment

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

Thanks @lewisl9029 , this should fix bunch of packages built by webpack! change looks great to me!

@@ -1200,7 +1200,7 @@ impl CJSLexer {
}

if webpack_require_props == 2 {
stmts.iter().skip(first_stmt_index + 1).find(|stmt| match stmt {
stmts.iter().skip(first_stmt_index + 1).take(8).find(|stmt| match stmt {
Copy link
Member

Choose a reason for hiding this comment

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

nice take!

@ije ije merged commit d936477 into esm-dev:main Jan 16, 2024
1 check passed
@lewisl9029 lewisl9029 deleted the detect-minified-umd-with-fn-decls branch January 17, 2024 01:42
@ije
Copy link
Member

ije commented Jan 17, 2024

Upgraded swc crates and bumped the version to 0.10.0, thanks again @lewisl9029 ❤️

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