search

magesuite / magepack

Next generation Magento 2 advanced JavaScript bundler.
Open Software License 3.0
431 stars 93 forks source link

Correct detection of anonymous AMD modules #142

Closed fredden closed 1 year ago

fredden commented 2 years ago

Fixes #140. Fixes #154.

The existing / old regular expression is finding this on line 35: return $.effects.define( "fade", "toggle", function( options, done ) { and matching define( " and treating this as a named AMD module (called 'fade'), however it should have been looking for / found this on line 22: define( [ and treated this file as an anonymous AMD module and therefore named it in the bundle.

This pull request corrects the regular expression so that line 22 is found / observed and line 35 is ignored.

krzksz commented 1 year ago

Hey @fredden, thanks for contributing. Unfortunately I didn't notice your PR before pushing my fix (I don't have much time to spend on Magepack anymore). I also added additional tests and my solution is a bit different so I hope you don't mind.

fredden commented 1 year ago

Hi @krzksz. Thanks for your update. Please can you re-open this pull request; I don't seem to have the permissions to do so myself.

my solution is a bit different

I've found 3ecd854697b2c29f717293416ee6b2e2c45641a1 which is what I assume you're referring to. This looks like a great start, but doesn't solve the problem. It does solve the one case that's been reported, but won't fix cases where other syntax is used which happens to (still) match the regular expression (like SomeObject.redefine('hats', ...).

I also added additional tests

Thanks for the tip about the tests. When you've reopened this pull request I'll update the tests to cover more cases so this can be resolved properly. (If I modify the branch before you reopen this, then (I think that) GitHub will force us to create a new pull request instead.)

Unfortunately I didn't notice your PR before pushing my fix (I don't have much time to spend on Magepack anymore).

I'm sure there are people who are willing to help maintain this project (like myself). Feel free to open a discussion on GitHub or reach out individually if you'd like to discuss how that might work.

krzksz commented 1 year ago

Nice, opening right away! Having tests is critical for me right now as it makes testing and fixing things much quicker.

fredden commented 1 year ago

@krzksz I've worked out how to run the test suite (yarn run jest), and added tests to cover this change. I confirmed that the default branch fails with these new tests. I have also opened #160 to run the test suite in a GitHub Action (like is done for eslint) going forward.