Getting "require() must have a single string literal argument" when bundling moment.js

[TikiTDO]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

When attempting to package a project which makes use of the https://github.com/moment/moment.js library, the packager crashes with the error require() must have a single string literal argument

The library line in question is: https://github.com/moment/moment/blob/develop/moment.js#L1830 which appears to be perfectly javascript code.

This behavior does not seem to happen when I build the bundle with RN 0.48.4, however that particular build has issues when running on an phone running Oreo.

I can work around this issue by disabling the error in question by adding a return statement before this line: https://github.com/facebook/metro-bundler/blob/master/packages/metro-bundler/src/JSTransformer/worker/extract-dependencies.js#L39

If the current behavior is a bug, please provide the steps to reproduce and a minimal repository on GitHub that we can yarn install and yarn test.

I am working on a repo to reproduce this behavior, however my time is limited due to pressing issues that need to be addressed within the next week so I have not been able to get a proper minimalist reproduction of this behavior.

I will update this issue with a link once I can get a good example.

What is the expected behavior?

There should not be any errors when packaging the JS bundle with the above library

Please provide your exact metro-bundler configuration and mention your metro-bundler, node, yarn/npm version and operating system.

RN: 0.49.0-rc.5
metro-bundler: 0.13.0
OS: Arch Linux
Mobile OS: Android 8.0.0

[richardgirges]

I'm reproducing this as well:

• RN 0.49.0-rc.6
• metro-bundler 0.13.0
• also using moment.js

[richardgirges]

There's a PR open in moment.js that should address this issue:
moment/moment#4187

EDIT: To be clear, that PR addresses the issue from the moment.js side of things. Wondering if this may be an issue for other popular packages using dynamic require statements?

[tqc]

Dynamic require statements seem to be pretty rare. The only other one I’ve run into was realm, with a workaround added in realm/realm-js#1346. If you need a quick workaround, renaming the require function seems sufficient to avoid the error.

[cpojer]

Seems like there is a PR upstream for moment, so closing here.

[TikiTDO]

@cpojer The PR referenced above introduces some fairly major changes, and an entirely new level of abstraction to an extremely popular and widely used lib, all to get around an issue that is exclusive to a particular packager used almost exclusively with react-native. Thus far not a single maintainer from that lib has commented on the PR, and it strikes me as the type of change that would get rejected pretty quickly without a good bit of justification.

Is there a particular reason that the bundler needs to extract dependency information from code internal to another library? It seems a bit strange that this causes a hard error, especially when the affected code itself mentions that there is a fairly trivial workaround involving renaming the require method to literally anything else.

[tqc]

@TikiTDO Not sure what major changes you are referring to - diffs involving renames can be deceptive, and the code at issue in moment has had ‘todo: find a better way to do this’ on it for a while.

The hard error is presumably because otherwise calling the dynamic require would be a runtime error.

The quick workaround I mentioned works for realm, but isn’t such a good idea for moment, since in that case there is no guarantee that the invalid require won’t be called - it doesn’t fix anything, just bypasses the check.

In any case, there is now a trivial fix from the rn app side - you can add moment as a git dependency pointed to a specific commit.

[TikiTDO]

@tqc I personally don't mind the change, it's not too different from what I would do, and I'm already using that code in my project as a workaround. However, just the fact that you're introduced an entirely new file to the project, and moved a good few functions into there would definitely raise alarm bells if I was a maintainer.

If the PR you submitted had an official collaborator confirm that they approve this approach, I wouldn't have any issue at all. However up to now that entire discussion page is full of normal users, so I'm worried that this will just remain as a git reference in my package.json forever.

Also, it is entirely possible to use dynamic requires in some contexts as long as the module is explicitly required elsewhere as a hint to the bundler, and you enable require by verbose names (Which metro-bundler decided must be locked away behind __DEV__). This is how I've worked around this issue before; as part of my initialization code I would simply call a file that included all the locales that my app supports, and then the require statement in question worked just as well as any other. That is enough to ensure the code is included in the bundle, and from that point on there is no difference whether the name passed to require is generated dynamically, or is provided as a string constant.

What more, the code in question explicitly handled the failure case of dynamic require throwing an exception, which is a good practice if you're doing dynamic requires. In my experience, that's a much more effective way of dealing with this type of platform dependent behavior, as opposed to a packager deciding that this is a hard error without giving developers any way around it (short of editing module code).

[tqc]

@TikiTDO The whole thing is a few days old at this point, so I’m not to worried about maintainer response - nobody’s said it’s wrong either, and the horrible diff is entirely for backward compatibility reasons.

Are you sure about the dynamic requires working? In the case of moment locales, including them elsewhere will register the locale, and it will never attempt to add them dynamically. From what I’ve seen in webpack generated code, require calls get converted to an integer which won’t match a generated string, and I’m sure I’ve seen references to the names not existing at all in production builds. And that’s before you get into more advanced stuff like inlining and tree shaking.

I do agree that the hard error is annoying, but from a packager perspective there are definite advantages to being able to know at compile time exactly what code is being used.

[TikiTDO]

@tqc The last official commit to moment was in early August, so it's entirely possible that no maintainer has even looked at the issues in the past few days. Hopefully you're right, but I've long ago learned to assume the worst in this profession.

In regards to dynamic requires: the way metro-bundler works in my experience, is it tries to convert absolutely everything it can to integers. This is true for both development and production modes. However, in development mode the require and define polyfills expose the functionality to require modules by string name, which uses an intermediate object to map string names to integers.

For reasons I don't entirely get, metro-bundler decided to restrict this functionality to development mode only, but I've actually used a development bundle in production without too many issues. That said, I've found that there's very little performance gain to be had on requires since they tend to run fairly infrequently, and I've found that the changes to the size of the bundle are negligible in a project of any significant size.

Incidentally, even if performance was a problem it would be very trivial to generate transformers that could selectively call an integer or a generic require based on what is being passed into the function. If you were to combine such a feature with the ability to translate unresolvable require parameters into strings, this entire conversation could have been avoided at the cost of having to implement an extra function for a somewhat rare use case.

In terms of the more advanced features you mentioned, the fbjs-inline-requires are done at compile time using babel-visitors, and seems to only apply to top level requires in the context of a single module at a time, so it should not really affect the use case in question. This can also be done regardless of whether a module requires an integer or a string. In any case, this specific use case seems to mostly revolve around optimizing fractions of a second from app start times, which can make a difference in automated testing, but is not extremely visible in production.

The dead code elimination algorithms might pose a bit more of a problem, but I have found the babel dead code elimination algorithms to be quite conservative, so it probably wouldn't remove any of the code that would get required by name, simply because the act of requiring may in fact have global effects outside of the scope that a parser can easily analyze.

Also, I do understand that the packager has different requirements and constraints that might not be interesting to a normal user. However, there's still the fact that this is all happening in a very large ecosystem. If doing it from basic principles is really what the doctor ordered, then perhaps the proper approach would be to go the Google route and really do everything from scratch starting with a whole new language. When a framework is hyped on the fact that it is compatible with a plethora of mature libraries, then I would expect some level of consideration to be given to the fact that some of those mature libraries may not align with the style that a particular packager might find easier to parse.

At the very least it would be nice to be given warnings and choices, as opposed to being forced into having to chose between multiple fatally failing scenarios, and spending valuable development time digging through unrelated libraries because someone decided a use case can not be valid. This is hardly the first time I've had to spend hours and hours reading through and debugging the code from this project, and the fact that it's extremely parallelized, and not very well documented when it comes to why and how things are done doesn't make the process easy or fun. At this point I am far more familiar with this bundler than I have any right to be, more so than any other similar project in any of the other platform that I've used over the past couple of decades.

The fact that almost every time I reluctantly open up this code base I end up seeing yet another decision that was made to simplify packaging at the expense of otherwise valid use cases really goes beyond merely annoying.

[cpojer]

I think we'd be ok with optional dependencies where require is directly within a try block (no nesting etc.). If you'd like to go ahead and make this change to metro, and send a PR and tests with it, we'd greatly appreciate it.

The place to make this change would be here: https://github.com/facebook/metro-bundler/blob/master/packages/metro-bundler/src/JSTransformer/worker/extract-dependencies.js#L45

The open source repo is in a bit of a rough shape (we'll fix that in the next few months and spend a bigger amount of helping the community make contributions), but if you could make the change, please add a test to the coressponding test files and don't introduce more flow errors and such, and I'm happy to review and land it. Please cc me on the PR. Thanks!

[cpojer]

@TikiTDO It would also be awesome if you could channel your energy into helping us make the open source workflow for this project better. Please consider the possibility that we are actually working on other things that are more important to us (or the company that pays our salary ;) ) at the moment than optional dependencies. Also note that this is free software, so it's up to you to use it and make it work for your use case and there is no guarantee that you'll either receive support or that we'll enable certain use cases for you. However, I'm excited about the possibility to change all of this for Metro specifically, and am hoping to receive more help from the community to get this repo into a great shape, document it and make it much more approachable to our users in open source. We can only do it together with you, and depend on your help to make this happen.

As a start, @mjesun has written up the first rough page of documentation that will sync to this repo soon. Further, we'll be rewriting/replacing significant portions of Metro over the coming months and remove a ton of old code to make this repo easier to work with and to set it up for the future.

[mauron85]

This definitely should not be closed as resolved with reference to unmerged PR for momentjs. What about other libs? Please fix metro bundler. Don't stick head into sand!

[TikiTDO]

@cpojer I would be happy to help, assuming I have the banwidth and the go-ahead from devs to do so. However, the original solution of closing the issue did not seem to offer that as an alternative. I had some ideas on how to do so within my post, and given time to percolate I could probably find a nice middle ground that lets even more things though.

Unfortunately my past few weeks have been spent chasing fire after fire, with hard deadlines looming ever closer, and clients breathing down my back to get it fixed yesterday. I've also had some negative experiences with submitting PRs that have taken me many hours of work, only to be rejected for not following some sort of implied conventions. I'm loathe to begin another batch of work without hearing back from maintainers confirming that an approach I try is valid for the project.

I should have some time Thursday evening write up the require within try/catch. Also, I would recommend having either an ENV variable, or an rn-cli.config.js variable to replace this hard error with a console warning. Such an option would have helped me get around the issue when I needed my app to work on an Oreo phone with debugging ASAP.

In the longer term, I would like to propose a solution that partially enables require by string in certain situations. I was thinking of having a transformer that would rewrite those requests to require._byString("abitrary/string/" + variable). Then with some level of config it would be possible to either:

1. Write a callback that receives the babel nodes, and return a list of all possible dependencies that it may translate to

2. Store the string names of defined modules in production , to enable above functionality explicitly