search

montagejs / mop

Montage Optimizer (mop): Minifies (to reduce file size) and creates bundles (to reduce the number of requests) of Montage applications.
Other
31 stars 15 forks source link

Dependency logic does not skip over Javascript comments #51

Closed rkaiser0324 closed 6 years ago

rkaiser0324 commented 10 years ago

If a require() exists in a comment it should be ignored for the purposes of building, but it's not. E.g.,

/* this should be ignored
require('/lib/foo.js');
*/

But when you build it you'll get a runtime error looking for foo.js.

Stuk commented 10 years ago

Mr uses a naive require parser for speed in the browser. In Mop we could do a real parse and AST traverse using Esprima.

kriskowal commented 10 years ago

We could also filter the dependency list for non-extant files.

kriskowal commented 10 years ago

@stuk, the way I see this issue is that

  1. at the moment, we can't use Esprima because in typical usage, the module loader is included in the bundles, even for production.
  2. in the v2 timeline (which has very little relevance on Montage in the near term), we could reasonably migrate to using Esprima for static analysis, on the supposition that Mr would almost never be needed in bundles for production.

Or, we keep to the status quo indefinitely.

What do you think?

Stuk commented 10 years ago

I need to check to see if an error happens when mopping with a commented require. If no error occurs then I'm fine leaving this case as is for the moment, and improving the documentation to make note of it.

If this issue gets more +1s then we can spend some time making parseDependencies work with Esprima in Mop.

Stuk commented 10 years ago

Related to https://github.com/montagejs/mr/issues/12

hthetiot commented 6 years ago

Fixed in mr