Getting "require() must have a single string literal argument" when bundling moment.js #65
Comments
|
I'm reproducing this as well:
|
|
There's a PR open in moment.js that should address this issue: 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? |
|
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. |
|
Seems like there is a PR upstream for moment, so closing here. |
|
@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. |
|
@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. |
|
@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 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). |
|
@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. |
|
@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 For reasons I don't entirely get, 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. |
|
I think we'd be ok with optional dependencies where 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! |
|
@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. |
|
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! |
|
@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
|
|
You can probably already do |
|
Metro Bundler is now breaking almost every RN version upgrade. I see same fatal error with |
|
[Removed] (I was experiencing a different problem than the issue posted, but receiving the same error.) |
|
I share @fungilation's sentiment -- metro bundler has broken every RN upgrade we've tried since it was released. How is it that you can introduce breaking changes like this (and others) without any warning, leaving the community to try and come up with workarounds? It's a nightmare. Guys, this breaks moment.js, used in just about any js/RN app you can think of. Furthermore, @tqc has a PR that doesn't work, it breaks our app in many places. It's not a trivial fix to this major lib. Provide a workaround that doesn't take 3 hours to troubleshoot, or revert the changes. |
|
Before this devolves into nastiness. I'm not saying we don't appreciate contributors' time and work in this as FOSS. But don't push changes that's not well tested and can, and does, break shit when the better option is slow down with changes until they are well tested. Alongside RN releases as there's a pattern of continued breakage. |
|
@sjmueller I’d appreciate a comment on moment/moment#4187 as to the form of import that is breaking for you - my updates there don’t change the interface beyond the dynamic locale loader itself, but this is testing the es6 version of moment rather harder than anything has previously. |
|
The things I've tried so far to get our app working again:
All out of options, what can I do now? |
|
@sjmueller All the workarounds up to now have had to do with fixing the build error during bundling, not necessarily ensuring all code is properly working in all scenarios. If your issue is that moment.locale is undefined, then you need to explicitly import the locales you want available before you first use moment. The act of importing a locale will actually make it available to moment internally. |
@sjmueller I downgrade RN back to 0.48.4 by reverting git changes, nuke |
|
@TikiTDO Turns out moment also has a few difficult bugs involving locales and ES6. They get imported using an explicit path, which conflicts with the idea of a separate entry point for ES6/RN. Making it work for RN would be trivial, but not without breaking a different module system. Is there anything in the bundler settings that would allow aliasing moment/locale to moment/src/locale? |
|
@tqc The locales in moment aren't really written as modules in their own right. Importing a locale actually calls a function on moment to register the locale internally, so as long as this line can resolve the main moment library, everything should work. This is why something like That said, there is a babel plugin called module resolver, which can help set up aliases (I actually use it to support symlinks in metro-bundler), however it's non-trivial and super finicky, so I would not recommend it as a generic solution. It's also possible to play around with a project's |
|
Yes the problem is back with react-native 0.52.0 and momentjs 2.20.1 EDIT: |
Note the bundler for React Native will do minification anyway when you generate a production bundle. |
Yes but the minified js that moment ships are different from the not-minified. |
|
I realised a possible solution for function getSmth(name) {
return require('/smth/' + name + '.js')
};In Metro this will cause an error because this is not a static string. A solution is to provide an alternative implementation of {
"name": "some-package",
...,
"react-native": {
"./lib/dynamicDeps.js": "./lib/dynamicDeps_forRN.js",
}
}Then, function getSmth(name) {
throw new NotAvailableError('Smths are not available with React Native');
};Thanks to the |
|
Please, don't pollute other packages with hacks because metro has inherently broken design assumptions! I'd prefer if metro doing one of two things:
Current metro behavior simple incorrect... |
|
@vovkasm Other packagers such as webpack actually convert module string names into numeric indexes in production mode; it offers approximately a 10-20x performance boost to do so, which can be significant in some situations. The idea of meta-info is something I suggested originally when I reported this issue. In fact if you take a look at the require and define polyfills, they do support this functionality in development mode. However, it's explicitly disabled for production mode (most likely for performance reasons) |
|
@TikiTDO Yes your are right, webpack solves this with context modules. But this not change my opinion. Metro should not force library autors to some special support. webpack solves this problem, metro can (actually should) also. It can be solved in such way that not reduces performance or bundle size if dynamic require not used, right? |
|
@vovkasm Unfortunately, it does not seem that this is a sufficiently high priority issue for the maintainers to devote their own resources to. It would not be extremely difficult to modify the methods I cited to work in production (though it would need some babel modifications in addition to that). Whether such modifications meet the design constraints of the maintainers is something I can not comment on. |
I'd like to nuance this a little bit: there are lots of JavaScript packages on So what I was trying to say is, lightweight workarounds for metro/react-native are either using the |
|
I was able to solve my issue by upgrading to the newest version of Moment, but for those who are still looking for a solution and/or are unhappy with how Metro bundler is working out for them on their project, it may be worth looking into Haul, which is RN bundler built on Webpack. I haven't tried it out yet myself, but I've heard good things. |
|
Alright, mitigation on the way. We'll make it so dynamic requires from within |
Summary: Tries to adress facebook/metro#65. We need a reasonnable workaround to support modules like `moment.js` that do dynamic requires but only in some cases. By replacing the call by a function that throws, we move the exception at runtime instead of happening at compile time. We don't want to do that for non-node_modules file because they are fixable directly, while `node_modules` are not fixable by people and they get completely blocked by the error at compile time. Reviewed By: rafeca Differential Revision: D6736989 fbshipit-source-id: a6e1fd9b56fa83907400884efd8f8594018b7c37
Summary: Tries to adress #65. We need a reasonnable workaround to support modules like `moment.js` that do dynamic requires but only in some cases. By replacing the call by a function that throws, we move the exception at runtime instead of happening at compile time. We don't want to do that for non-node_modules file because they are fixable directly, while `node_modules` are not fixable by people and they get completely blocked by the error at compile time. Reviewed By: rafeca Differential Revision: D6736989 fbshipit-source-id: a6e1fd9b56fa83907400884efd8f8594018b7c37
|
@jeanlauliac That is great! I tried the latest metro code from master and I am getting an compile time error: |
|
I think you cannot use master directly, because it's not precompiled and setup properly. Can you try |
|
@jeanlauliac Yes, it is working with metro@0.24.6. Thank you! |
|
I'm getting the same error running react-native 0.52.2 and metro-bundlner 0.22.1, but I can't tell which library is causing it. How do I figure out why this is happening? I tried to specify metro-bundler 0.24.6, but yarn won't let me. |
|
Howdy! Normally this should be mitigated in the last version of Metro: packages in We don't have immediate plans to make dynamic requires always work at the moment, but for your own code, note it's possible to workaround the issue by using a series of explicit requires. For example, imagine I want to list language packs: const languages = {
'en_US': require('./en_US'),
'en_GB': require('./en_GB'),
'zh_CN': require('./zh_CN'),
// etc.
};You could generate such listing using a script, mitigating the problem of having to maintain these by hand. As such, for the time being, I'll close this task. Feel free to open a new task to discuss a specific new concern or problem that may happen! |
|
@jeanlauliac I'm not sure that I have the courage to read all 164 comments, but I don't think it's good idea to close the issue just saying "We don't have immediate plans to make dynamic requires always work at the moment". The problem is not solved, the problem persists, and the fact that you don't have immediate plans to solve it, doesn't mean that we can just close the issue and forget about the problem. |
|
Of course @vyshkant , I think the open task #52 is a good place to discuss about dynamic requires in general. It's my understanding this thread was more about the specific problem caused by the inability to compile moment.js at all, not the lack of a dynamic require feature. The inability to use moment.js has has normally been mitigated in the latests version of Metro and React Native. (If not, let's reopen a task, by all means.) Does that sound good? |
Summary: Tries to adress facebook/metro#65. We need a reasonnable workaround to support modules like `moment.js` that do dynamic requires but only in some cases. By replacing the call by a function that throws, we move the exception at runtime instead of happening at compile time. We don't want to do that for non-node_modules file because they are fixable directly, while `node_modules` are not fixable by people and they get completely blocked by the error at compile time. Reviewed By: rafeca Differential Revision: D6736989 fbshipit-source-id: a6e1fd9b56fa83907400884efd8f8594018b7c37
Basically, the `metro-bundler` never accepted the way moment worked with dynamic imports. But 2.19.0 fixes that and allows the lib to be used on RN >=49.0. Read here for [more details](facebook/metro#65 (comment)).
Basically, the `metro-bundler` never accepted the way moment worked with dynamic imports. But 2.19.0 fixes that and allows the lib to be used on RN >=49.0. Read here for [more details](facebook/metro#65 (comment)).
Basically, the `metro-bundler` never accepted the way moment worked with dynamic imports. But 2.19.0 fixes that and allows the lib to be used on RN >=49.0. Read here for [more details](facebook/metro#65 (comment)).
Basically, the `metro-bundler` never accepted the way moment worked with dynamic imports. But 2.19.0 fixes that and allows the lib to be used on RN >=49.0. Read here for [more details](facebook/metro#65 (comment)).
and others
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 argumentThe 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
returnstatement before this line: https://github.com/facebook/metro-bundler/blob/master/packages/metro-bundler/src/JSTransformer/worker/extract-dependencies.js#L39If the current behavior is a bug, please provide the steps to reproduce and a minimal repository on GitHub that we can
yarn installandyarn 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
The text was updated successfully, but these errors were encountered: