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

Implement the "noParse" matcher via mdeps "parseFilter" #1256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zertosh
Copy link
Member

@zertosh zertosh commented May 9, 2015

A proposal based on the discussion in browserify/module-deps#54 (comment). Depends on browserify/module-deps#87. don't merge this - the package.json is point to my module-deps branch for this

Currently the logic for determining whether to parse for deps or not lives in module-deps. Browserify passes an array of absolute paths to module-deps, and module-deps does a straight indexOf against it. A couple of things feel off about this:

  1. The pattern matching for exclude, ignore, and the globals transform are each different than noparse and among themselves.
    • This PR begins to address this by isolating the globals transform's file matcher into a fileFilter function (idc about the name) for use by a new parseFilter available in module-deps.
    • With this logic isolated, we can apply it to exclude and ignore.
  2. module-deps doesn't have logic for any kind of path matching - it delegates to "filters".
    • The approach proposed (parseFilter) follows in the footsteps of postFilter, filter and packageFilter.

Things that "break":

  • It's undocumented but you can currently do b.require('thing', {noparse: true}); - that wouldn't work anymore.
  • module-deps would lose the ability to noparse via the CLI - but that's also undocumented. If this is important, we could implement it in it's cmd.js.

Follow ups:

  • fileFilter, mostly taken from the globals transform is broken - PR coming soon.
  • noParse doesn't have any tests. About half of the ones here fail against master (bc of new functionality). I'll make a separate PR for the passing ones, so this change doesn't introduce currently unknown regressions.

@jmm @terinjokes

@substack <-- I don't know if you like being tagged on these things or not

@zertosh
Copy link
Member Author

zertosh commented May 9, 2015

#1257 is the PR from the "follows up" -- I'll have to rebase once that's merged.

@jmm
Copy link
Collaborator

jmm commented May 13, 2015

It's undocumented but you can currently do b.require('thing', {noparse: true}); - that wouldn't work anymore.

As I said, I like the idea of making path-matching that happens in various places consistent (where that makes sense in context). But at this point I'm not crazy about ditching row.noparse. I think that could be useful. In fact, I used it here: browserify/module-deps@3.7.2...jmm:pragma (part of my proposal in #1151). Is there some compelling reason to get rid of it?

Getting rid of both row.noparse and this.option.noParse means that even for module-deps itself to noparse something would require wrapping parseFilter(), right? And if someone is using module-deps independently of browserify it also seems like a bit of a pain (and inconsistent with browserify) to have to do:

var noParse = [
  // ...
];

mdeps({
  parseFilter: function (id, file) {
    return noParse.indexOf(file) < 0;
  }
})

// As opposed to:

mdeps({
  noParse: noParse
})

If adding parseFilter() helps with the consistency problem, then what about adding it but leaving the other things? And when invoking module-deps from browserify, just use that instead of opts.noParse. Or even overload opts.noParse to be able to accept a function as browserify seems to (undocumentedly) do.

@zertosh
Copy link
Member Author

zertosh commented May 15, 2015

Is there some compelling reason to get rid of it?

I want to get rid of it because there are too many things called "no(P)arse" that do very different things: (1) don't look for deps, (2) don't apply the globals transform. I'd propose to rename it, or break it up, but that'll make things worse. There's already a mismatch in case in module-deps: the option passed in has to be "noParse", but the value on the row has to be "noparse".

But maybe it would be too much of a breaking change to remove it from module-deps. I'll update this PR and the one in module-deps so only parseFilter is a thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants