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

$' combination breaks parsing #32

Closed
vladimir-tikhonov opened this issue Feb 27, 2017 · 9 comments
Closed

$' combination breaks parsing #32

vladimir-tikhonov opened this issue Feb 27, 2017 · 9 comments
Assignees

Comments

@vladimir-tikhonov
Copy link
Collaborator

vladimir-tikhonov commented Feb 27, 2017

Hello,

I've noticed that inject-loader cannot correctly process $' combination in files.

Given the following code:

define(function (require) {
    'use strict';

    var _ = require('lodash');
    var regexp = new RegExp('^(one|two|three)$', 'i');

    return {
        getRegex: _.constant(regexp)
    };
});

I've got the following output:

module.exports = function __injectWrapper() {
    var __injections = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : {};

    var module = { exports: {} };
    var exports = module.exports;

    // $FlowIgnore
    var __wrappedModuleDependencies = ["lodash"];

    (function __validateInjection() {
      var injectionKeys = Object.keys(__injections);
      var invalidInjectionKeys = injectionKeys.filter(function (x) {
        return __wrappedModuleDependencies.indexOf(x) === -1;
      });

      if (invalidInjectionKeys.length > 0) throw new Error('One or more of the injections you passed in is invalid for the module you are attempting to inject into.\n\n- Valid injection targets for this module are: ' + __wrappedModuleDependencies.join(' ') + '\n- The following injections were passed in:     ' + injectionKeys.join(' ') + '\n- The following injections are invalid:        ' + invalidInjectionKeys.join(' ') + '\n');
    })();

    function __injection(dependency) {
      return __injections.hasOwnProperty(dependency) ? __injections[dependency] : null;
    }

    /*!************************************************************************/
    (function () {
      // $FlowIgnore
      'use strict';

define(function (require) {
    'use strict';

    var _ = (__injection("lodash") || require("lodash"));
    var regexp = new RegExp('^(one|two|three)
    })();
    /*!************************************************************************/

    return module.exports;
  }, 'i');

    return {
        getRegex: _.constant(regexp)
    };
});
    })();
    /*!************************************************************************/

    return module.exports;
  }

This part is broken:

    var regexp = new RegExp('^(one|two|three)
    })();

Any questions - please let me know.
P.s. And many thanks for this package, it's a real life-saver for me 👍

@rmp135
Copy link

rmp135 commented Feb 27, 2017

I have a feeling this is due to the $' which is being parsed as a regex match when performing the injection. Pull request #31 refactored the code to use a string replace which should fix this issue.
In the meantime, you can use double quotes or string literals as a workaround.
I'll also write a test to cover this case.

@vladimir-tikhonov
Copy link
Collaborator Author

@rmp135 you are right, $' -> $" did the trick. Thank you!

@vladimir-tikhonov vladimir-tikhonov changed the title Parentheses in strings break parsing $' combination breaks parsing Feb 27, 2017
@vladimir-tikhonov
Copy link
Collaborator Author

vladimir-tikhonov commented Mar 13, 2017

If someone is affected by the same or similar issue - i just wrote injectify-loader which has the same api/behaviour, but using babel tools under the hood (instead of regular expressions). I was able to just replace inject-loader with injectify-loader in my project - and everything was working like a charm.

@plasticine
Copy link
Owner

@vladimir-tikhonov Hey, that’s pretty cool—nice work! I’d kinda been meaning to explore something like that for this module given the ubiquity of babel these days (was not the case as much back when I first wrote inject-loader), and the hackiness of using regex. Would you consider a PR?

@vladimir-tikhonov
Copy link
Collaborator Author

@plasticine I've written injectify-loader from scratch, so the only type of pr I can submit is "remove everything from inject-loader / add everything from injectify-loader" 😄

@plasticine
Copy link
Owner

@vladimir-tikhonov Heh, fair enough :simple_smile: I suspect the next time I get a chance to do some OSS stuff I’ll probably be looking at going in the same direction in this project anyway, but I can certainly understand that you might want to keep injectify-loader as a separate project. 👍

In the meantime, @rmp135 is correct in stating that his PR #31 should resolve the above issue.

@vladimir-tikhonov
Copy link
Collaborator Author

vladimir-tikhonov commented Mar 14, 2017

@plasticine I just wanted to state that these projects have many differences, so if I submit a pr it will be a complete rewrite of this package. However, I can definitely do this, if you are ok with that. Moreover, I think that having only one package that does the right thing is much better for the js community.

@plasticine
Copy link
Owner

@vladimir-tikhonov nah, I would not be opposed to that at all — this project has never really been a very complex idea or implementation, and your implementation is really great.

I would love to be able to rule out a whole category of bugs (i.e this one) that come from hacky regexes, by adopting babel to transform code instead (it may also allow for fixing #17 finally!). I think adopting babel for this task is really probably the right move.

If you’re willing, I’d love to work on getting your improvements across.

@vladimir-tikhonov
Copy link
Collaborator Author

@plasticine okie dokie, I'll submit a pr in a couple of days

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

No branches or pull requests

3 participants