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

Put fs on its own line #108

Closed
wants to merge 1 commit into from
Closed

Conversation

RichardLitt
Copy link

Due to the way the brfs transform works, I was having a breaking error with fs being part of a multiple variable definition (see this. This change fixes it.

Due to the way the brfs transform works, I was having a breaking error with fs being part of a multiple variable definition (see [this](browserify/brfs#28). This change fixes it.
@mscdex
Copy link
Owner

mscdex commented Feb 17, 2015

IMHO this is something that should instead be fixed upstream in brfs, not worked around in each individual module that uses a coding style like this. That brfs issue shows just how more widespread the issue is, so it really makes more sense to fix it at the source.

@mscdex mscdex closed this Feb 17, 2015
@RichardLitt
Copy link
Author

I agree. However, I can't use some modules (mailgun-js) that use ftp at the moment because this issue is breaking my build. I'm not sure asking me to manually change the file until brfs fixes it is the best thing, either... What do you think? It isn't a breaking change to commit to ftp.

@mscdex
Copy link
Owner

mscdex commented Feb 18, 2015

Ok, so the problem is further upstream in static-module. I've submitted a failing test to that repo here.

@RichardLitt
Copy link
Author

Nice! I spent a couple of hours trying to find a solution for it in static-module, too, but I didn't understand the output of falafel enough. Good work. Hopefully this'll get somewhere. For now, in my own environment, I'm using an npm-shrinkwraped version of the module that I have that calls ftp and point to my patch instead of here.

@mscdex
Copy link
Owner

mscdex commented Feb 18, 2015

I tried looking into it briefly, but I'm not familiar enough with the code base and how the logic flows. Based on my limited debugging though, my hunch is that the problem lies somewhere in here and it may be that either the offsets in the code are not calculated correctly or the order in which the string replacements occur is not right (possibly causing offsets to be stale/incorrect?).

@RichardLitt
Copy link
Author

Having just spent another few hours on it; I've got no idea. I think it's got to do with semicolon removal. But really, I've got no idea.

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