Dep round trip #1415
Dep round trip #1415
Conversation
Browserify-incremental makes the assumption that we can round-trip `dep` events (slightly mutated to form an object with `row.file` as the key) back into the cache with no ill effects. This is true except for the edge case shown here - when you are both exposing a module and requiring it in the same bundle.
It should be safe to not walk down an exposed module, since it's being taken care of at the top-level. If we don't do this, `module-deps` tries to open a file using the module's exposed name rather than a valid filename, which errors out. This is only triggered by passing in a cache value that should be valid.
|
@thedufer Thanks for contributing to browserify! I haven't looked at your PR enough to comment on anything else, but re:
That actually is not documented. In my opinion nothing related to that should be changed without it first being documented. I think it's intended to work that way, and therefore should be documented, but I've gotten no feedback on my issue. |
|
We definitely depend on that behavior, although I do now remember that we had to do some testing to make sure it did what we hoped, because of the lack of documentation. I agree that it should be documented. Anything I can do to help? What kind of feedback would you want on that issue before its worth fixing up the documentation? |
Probably not, unfortunately, but thank you for asking. 😃
It's definitely worth fixing (if there's something to be fixed). I'm looking for feedback from @substack or at least a couple of other Collaborators. I can make a guess, even an educated one, on my own about how it's intended to work, but that's a haphazard basis for changing API documentation (and that can be tantamount to changing the API itself, which as far as I know no Collaborator should be doing unilaterally).
I'd give all the usual cautions about relying on undocumented behavior. People constantly want to do things that are undocumented and that do actually work the way they'd like, until it breaks in a future release (even a patch). See for example (they're all about the same undocumented behavior that lots of people were relying on): |
|
All of our build tools are carefully frozen to exact versions and we do thorough testing before pushing an upgrade to any of them. We're perfectly willing to fix things if this behavior changes; in the meantime, it's worth depending on this UB because we're getting significant performance benefits related to caching that we wouldn't be able to get without it. Thanks for the warning, though. |
This fixes jsdf/browserify-incremental#22, which arises because
depevents come with a row that can't be fed back into the cache. This happens in the case when you have a file that is both exposed and required under its exposed name in the same bundle (which seems like it ought to be valid, and works fine without setting the cache option).