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

Various small fixes #128

Merged
merged 6 commits into from
May 1, 2024
Merged

Conversation

anka-213
Copy link
Contributor

@anka-213 anka-213 commented Apr 29, 2024

Three fixes that prevented code I was working on from being unpacked

  • fix: Make isIIFE match what it claims to match
    More specifically:

    !function(){...}()
    

    wasn't matched, only

    !function(){...}
    

    which isn't an IIFE

  • Fix: Remove buggy and redundant transform

    No need to remove import foo as foo. The desired behaviour happens automatically and other code expects the local field (as foo) to exist to not crash.

    import { foo as foo } to import { foo }
    

    will automatically be printed as

    import { foo } to import { foo }
    

    without this transform

  • feat: Allow re-exports from modules in Webpack5
    If a module re-exports values from another module,
    Webpack5 would export them as

      var r = n(645); n.d(t, { foo: () => r.foo })
    

    so this adds member-expressions to the expected possible values

  • feat: Allow Arrow functions in module definitions

    Example:

    (self.webpackChunk_N_E=self.webpackChunk_N_E ||
         []).push([[888],{2189: (e,t,n) => {...} }
    

Example:
(self.webpackChunk_N_E=self.webpackChunk_N_E ||
 []).push([[888],{2189: (e,t,n) => {...} }
If a module re-exports values from another module,
Webpack5 would export them as

    var r = n(645); n.d(t, { foo: () => r.foo })

so this adds member-expressions to the expected possible values
Copy link

vercel bot commented Apr 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
wakaru ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2024 5:26pm

@pionxzh
Copy link
Owner

pionxzh commented Apr 29, 2024

Can you help to update the test cases accordingly?

@anka-213
Copy link
Contributor Author

Will do! I was apparently wrong in my assumption regarding the foo as foo. I thought I had tested it, but apparently my tests didn't cover those cases.

@pionxzh
Copy link
Owner

pionxzh commented Apr 29, 2024

I want to know more about the "buggy" part of foo as foo transformation. What would be the real-world issue? Semantic changes or wakaru throwing error?

@anka-213
Copy link
Contributor Author

anka-213 commented Apr 29, 2024

wakaru throwing error. More specifically it was when running wakaru all somefile.js. I got crashes from two of the transforms. Let me try to reproduce it.

I believe it's a bug in the scopes library from ast-types

@anka-213
Copy link
Contributor Author

These are the lines that the crash originated from:
https://github.com/benjamn/ast-types/blob/v0.16.1/src/scope.ts#L256-L258

As you can see it assumes that one of local, name or id exists on the binding, but if you call j.importSpecifier with only one argument none of them is set. When I tried using astexplorer, it does set local even if it's only a single value with no as.

@pionxzh
Copy link
Owner

pionxzh commented Apr 29, 2024

Do you have a short reproduction? We can also patch ast-types by ourself if this is indeed a bug from ast-types.

@anka-213
Copy link
Contributor Author

I unfortunately haven't extracted a short reproduction yet, but I have found a simple fix. Constructing a new identifier and using it for both arguments does produce the desired behavior.

I'll see if I can make a short reproduction as well.

@anka-213
Copy link
Contributor Author

I also removed another change that caused test failures. Sorry for not running the tests before sending a PR

@pionxzh
Copy link
Owner

pionxzh commented Apr 29, 2024

Don't worry.

I also updated another iife matching that should be corrected.

@pionxzh
Copy link
Owner

pionxzh commented Apr 29, 2024

one more thing. Please help to update the commit message from "Fix" to fix. The Changelog tool will be mad about it 🤣

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 65.62500% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 89.31%. Comparing base (e099371) to head (cc4a10a).

❗ Current head cc4a10a differs from pull request most recent head 14c4377. Consider uploading reports for the commit 14c4377 to get more accurate results

Files Patch % Lines
packages/unpacker/src/extractors/webpack/jsonp.ts 11.11% 8 Missing ⚠️
packages/ast-utils/src/matchers/isIIFE.ts 83.33% 2 Missing ⚠️
...ckages/unpacker/src/extractors/webpack/webpack5.ts 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
- Coverage   89.33%   89.31%   -0.02%     
==========================================
  Files         100      100              
  Lines       12308    12316       +8     
  Branches     1632     1635       +3     
==========================================
+ Hits        10995    11000       +5     
  Misses       1259     1259              
- Partials       54       57       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anka-213
Copy link
Contributor Author

Haha, done! I also restored the isIIFE fix now that your commit fixed the test failure.

@anka-213
Copy link
Contributor Author

anka-213 commented Apr 29, 2024

Here's a minimal reproduction of the bug. It's a combination of the first test case for un-import-rename and the first test case for un-optional-chaining, but it works with any combination of un-import-rename and something that uses @wakaru/ast-utils/scope.

import { foo as a, bar as b, code } from '_';

console.log(a, b, code);

var _a;
(_a = a) === null || _a === void 0 ? void 0 : _a.b;

edit: I would add it as a test, but I don't know how to add tests that requires multiple different transforms

@pionxzh
Copy link
Owner

pionxzh commented Apr 29, 2024

You can inline combined rules if there is only one test case, or declare it as a variable.

const inlineTest = defineInlineTest([unFlipComparisons, transform])

defineInlineTest([transform, unParameters])('SWC - ES5',
`
foo !== null && foo !== void 0 ? foo : "bar";

I would recommend you put it at the test of un-import-rename 🤔

pionxzh and others added 2 commits May 2, 2024 01:25
The scope module crashes when an import dosn't
have a local name
anka-213 and others added 2 commits May 2, 2024 01:26
More specifically
  !function(){...}()
wasn't matched, only
  !function(){...}
which isn't an IIFE
@pionxzh pionxzh merged commit 8546f81 into pionxzh:main May 1, 2024
3 checks passed
@pionxzh
Copy link
Owner

pionxzh commented May 1, 2024

Thanks for fixing some many issues. 👍

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