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

[gaearon]

Since you mentioned me:

Enough is enough.

I would suggest to notch the entitlement down a little bit. Yes, the situation is frustrating, but as you can see, it was caused by a mutual misunderstanding. The feature never worked in the first place. It is unfortunate that the strict error broke people’s projects, but there is already PR to fix it. Being antagonistic and patronizing in this thread will get you nowhere.

Regardless of the recruiting benefits for Facebook, I want to make it clear that open sourcing something (and then maintaining it at this scale) is always a matter of individual engineers’ commitment to open source. By being antagonistic you are stretching this commitment thin for each of us. Even those who joined from the open source world.

There is no top-down directive at Facebook to build open source software. In fact sometimes we (the engineers) choose to take a hit to our internal productivity and performance because we decide to go the extra mile for what we believe is right. This might be the reason our open source program is so successful: we don’t do vanity projects, and only share the code we heavily use to help move the industry forward. But every thread like this makes me want to do this less and less.

We make mistakes. It is impossible not to at this scale. With hundreds of thousands of dependants, something somewhere is going to break at some point. At the same time we juggle hundreds of other issues internally. It is frustrating if your code doesn’t work after upgrading. However, at the same time, due to today’s bugs, it doesn’t work on master for some teams at Facebook. If we don’t prioritize those problems, nobody at Facebook will want to use React Native, and the project will die. Obviously that would be even worse for the community longer term. So we have to balance the urgency of issues affecting FB and the community, and it is hard. We’re trying our best but I’m curious to know what strategy you would employ if you were in our shoes.

So please, stop with this demanding attitude. We make mistakes, and we are sorry. Sometimes we need help fixing them.

P.S. I’m not working on Metro or React Native. I’m speaking from my own experience, and don’t represent anyone else’s views. But since my employment at FB is being used to justify discussions in this tone, I figured I’d share my view.

[cpojer]

Diff landing now, we'll tag a new release of Metro today. You'll be able to use Yarn's selective versions resolution to overwrite the version of Metro used in your RN, see https://code.facebook.com/posts/274518539716230/announcing-yarn-1-0/

[cpojer]

@richardgirges thanks so much for digging in. I think you are on point, this is great. So the mismatch I had was that we "never supported dynamic requires", and at Facebook that is true because of the way we bundle our code (large parts of Metro are not actually open source yet). However, I did not realize that this piece of code didn't throw in the past and just silently skipped them, and also that open source is lagging like two months behind which means I just completely forgot we ever made a change in this code.

With all this it seems like #69 is the most productive fix to unblock everyone. I'll talk to @jeanlauliac and we'll see if we can merge it on Monday.

Just to give some background: There are multiple reasons why we do not support dynamic dependencies. First, static analysis breaks when dynamically requiring modules. Second, when using dynamic requires, the real module you are looking for may not be available. This can cause major issues in production that would prevent users from using our apps, like for example we could break Facebook. We had problems like this in the past and it isn't fun to deal with. Metro is intentionally opinionated to prevent people from doing things that will cause pain to users.

Finally, we understand this open source repo isn't in a great shape right now. The good news is that there have never been more people at FB working on Metro, and we have a number of major changes in store to make Metro a ton better, but the bad news is that there are pressing issues we need to work through unrelate to open source. We are hoping that in a couple of months this repo will be in a much better state and that it'll be easier to contribute to. Give us some time please.

[cpojer]

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

[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!

[deavial]

I have the issue as well now. Upgrade to 49 on an app that has been running perfectly fine since 38 and now I am stuck in the water. Can't do a thing because it won't play with moment. On top of that, the moment references are coming from 3rd party modules.

So what do I need to do? Downgrade? This should have been a yellow box until you knew how many developers were going to be taken offline.

[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?

[sjmueller]

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.

[sjmueller]

This is free software and there is no obligation on any one side to fix anything.

I actually don't agree with you at all @cpojer. Open source software is a business opportunity for facebook, for many reasons. If OS didn't have business benefits, facebook wouldn't be paying you to work on this software, plain and simple.

Specifically, fb has been able to recruit top talent in the industry because of their open source policy -- leaders in javscript alone like @gaearon & @kittens are a few great examples of fb reaping the benefits. Besides talent, fb is also in an amazing position to introduce a new mobile operating system/ecosystem that could legitimately challenge Android and iOS -- this is because boatloads of companies/engineers/organizations are using react native. Try climbing that mountain nowadays without releasing "free" software that gains massive traction first, and I guarantee you will be staring failure in the face.

So really just stop with the altruistic free software speak because the equation with open source isn't so binary. Furthermore, threatening to lock the thread because you're hearing the frustration of many developers is just poor judgement. People aren't mad just because of this one issue, they are mad because these major breaks are happening over and over and over and over again. Go back through my github history, the PRs I've submitted, the helping advice I've lent, the tone of patience and appreciation I've had in the past. I've tried to do my part as many have here, but it's just gotten ridiculous. Enough is enough.

[sebmck]

@sjmueller

So really just stop with the altruistic free software speak because the equation with open source isn't so binary

Keep in mind that the people you're accusing of not being responsive and receptive enough are taking part of their weekend to reply to you. 🙂

[peterjuras]

Any update on the new tagged release of metro-bundler?

[richardgirges]

@kelset all you have to do is yarn upgrade [email protected]. Don't upgrade metro-bundler as previously suggested. You can force all of the libs that use moment as a sub-dependency to use the newest version of moment as well by using Yarn's resolutions:

"resolutions": {
  "nameOfNodeModuleThatUsesMoment/moment": "2.19.0"
}

[albinekb]

I very much think not supporting dynamic require's like that is a good thing. But moment.js and all libs that people depend on that depends on moment.js is broken, and will probably not ever be fixed without replacing moment.js, and that's a lot of work..

so there's three ways as I see it:

• Support dynamic requires like the one in moment.js somehow
• Get a PR merged and a release of moment.js (if possible this would be the best)
• Everyone that uses moment.js (or other libs that do this) and want react 0.48+ need to change all their code to use a different lib

Maintaining a fork of moment would also work, but feels like a lot of work that are more fragile than the other options.

[richardgirges]

Frankly, @cpojer has been beyond accommodating. He listened to our opinions, reopened the thread at our request, and his responsiveness and eagerness to get this issue fixed has been way above average for an open source project. We have a PR that both sides seem to agree on so far, and @cpojer is trying to get this fix merged in as early as next week.

If you take all the drama out of this thread, you'd actually have a shining example of the community and the maintainers working together to reach a reasonable solution in a reasonable amount of time.

The irony is that I've seen way bigger blocker issues get resolved in a much longer timeframe in other open source projects. I'm not sure why this issue has to be the straw that broke the camel's back. OSS is a fickle game. Speaking as both a user and maintainer of OSS, it's a tough job for everyone and any amount of time invested in supporting the community is more than reasonable IMO. I've always seen it as a group effort.

To top that off, while we work on fast tracking a fix into metro-bundler, there are numerous workarounds to keep people going:

• Downgrade to RN 0.48.x
• Use @tqc's moment fork (assuming moment is the only dependency that's breaking your bundle)
• Manually hack extract-dependencies.js inside of metro-bundler (read thru @TikiTDO's original post at the top of this thread)

If people have an issue with the way an OSS project is maintained, perhaps there's a different forum for that (I dare not throw out a suggestion, that's up to the maintainers to clarify). Let's put the drama aside and stick to the issue at hand - we're so close to a working solution.

[jeanlauliac]

metro-bundler 0.20.0 is out, can you please try it out and lemme know if that mitigate the issue? :)

[fmaceachen]

@jeanlauliac Getting the same as @rcorrie
Unhandled JS Exception: Can't find variable: process

[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.

[fungilation]

It shouldn't even be a discussion regarding "waiting for a fix to land in moment". They have architectural issues they want to clean up surfaced by this dynamic import debacle but what really broke the camel's back is Metro Bundler. As Moment isn't the only thing that's now broken in RN 0.49 because of dynamic imports.

Dynamic imports is javascript. It worked before on RN 0.48 and prior. RN 0.49 broke it, so it's RN / Bundler's issue to fix, including reverting breaking changes if necessary.

[rcorrie]

@jeanlauliac
The bad news: I am still getting an error.
The good news: It is not longer the same one mentioned in this thread.

It is highly likely that this new error is unrelated to metro, and just some botched upgrade on my part 👍

Can't find variable: process

Update: seems like I'm not the only one with this error

[fungilation]

Metro Bundler is now breaking almost every RN version upgrade.

I see same fatal error with moment-timezone package since upgrading to RN 0.49.1

[gaearon]

Also:

If OS didn't have business benefits, facebook wouldn't be paying you to work on this software, plain and simple.

This statement gets the cause and effect exactly backwards.

Facebook pays us to work on these projects because they are useful. Not because open source has business benefits. There are hundreds of infrastructure projects at Facebook that we could work on instead that are just as useful, and don’t happen to be open source. Whether or not a project is open source has nothing to do with Facebook paying us.

The software in question is open source because the engineers who work on it firmly believe that this is a better way to develop, and because they wanted to share it with their peers. It is indeed very nice that it helps Facebook improve its engineering brand for recruiting, and that occasionally we get very valuable contributions from the community. But React Native, Metro, Yoga, etc, are open source because engineers who worked on them cared. Not because “Facebook pays us to work on open source”.

(Apologies for adding another dramatic comment, but I wanted to clarify this common misconception. Indeed, overall I think the thread was productive, but it could’ve been less emotionally charged.)

[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.

[fungilation]

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.

[tqc]

@albinekb my no-dynamic-import branch moment/moment#4187 now handles locale imports ok if you import them from /src/locales. The main problem now is where the moment import is in another module which you can’t control directly, which is running into some issues with the behavior of es6 default exports.

[richardgirges]

I think waiting for a fix to land in moment is a bit of a red herring. Even if a fix does land, it doesn't solve the issue of third party modules utilizing moment. I ran into this specific issue myself. It's time consuming and silly to have to track down every project I use that depends on moment and create forks of those projects left and right, in the hopes that they will one day update to a later version moment if a fix even makes it into moment.

A good example of this is validate.js which ships with it's own baked-in version of moment.js that contains the non-static require.

I don't think there's anyway around the more immediate fix manifesting in metro bundler.

EDIT: That's not to discredit the stellar work @tqc is doing on the moment side of things. I still see the value in that. Just saying that I hope we don't devalue the notion that a more immediate fix is necessary in metro bundler in the meantime.

[cpojer]

Alright, let's step back a little bit. This discussion is going off the rails. If you are having a problem with code in this repo, the only way to get it fixed is by working together and sending pull requests. This is free software and there is no obligation on any one side to fix anything. I know it sucks if things don't work well but if you are having a problem, instead of getting frustrated and angry on this thread I encourage you to contribute productively, otherwise I'll close this and lock the thread.

I'd like to get an actual understanding of this issue. Is there anything in RN that is linking to this issue and shifting the focus to Metro? Metro has not supported dynamic imports for the entire three years of its existence and I find it hard to believe that the recent RN upgrade changed anything here. It's more likely that there is something else that happened, that can be fixed easily, that will unblock all of you.

[albinekb]

Awesome @cpojer ! Could you provide an example of such config for the selective feature with yarn? 🤔 So I don't have to guess how it should be configured 😄

[gaearon]

I searched for “yarn resolutions” and found this:

https://yarnpkg.com/lang/en/docs/selective-version-resolutions/

[romreed]

I'm beginner in RN and not understand how fix this problem. Can some one show how i must configure project for it work? Thank.

[TikiTDO]

@albinekb I pursued that avenue of thought a bit, but unfortunately there are two big problems that make this solution non-trivial. First, API of the extractDependencies function currently only takes a string of code to be parsed, which would make it quite challenging to determine what specific imports you're dealing with. The parent function would need to be expanded to receive contextual information which a callback could use to make decisions.

Second, adding a callback here would not resolve the issue of actually doing the imports. Even if you could calculate the string dependencies at this point, and alias said strings to an integer module definition somewhere in the bundle, there's still the problem of actually handling the require.

The require in question would need to be transformed to call some sort of handler which would then be able to resolve a string to an integer, and do the require based on that. There is a similar bit of functionality currently available for development, but this would need to be adapted to production.

Finally, there is the political/stylistic question of what sort of implementation would be acceptable to the maintainers. I would be loathe to spend much time doing something like this without a well defined design, since doing it otherwise easily lead to a rejection and hours of effort wasted. However, given the current rate of commits to the project I do not think anyone from facebook has the bandwidth to write up a specification for this sort of behavior.

@tqc Assuming the https://github.com/tqc/moment/commits/rn49hacks test branch you made works, I think it will be safe. Any modules that use moment internally should be doing some variation of require("moment") or import "moment". This should then get compiled by metro-bundler to require(number) of the react-native version of moment at bundle time.

[tqc]

@TikiTDO That is the theory, yes. The actual behavior is something more interesting. It should be possible to make an ES6 module that behaves the same way as the ES5 module it compiles into, but so far that isn't happening. I don't think it's directly related to this issue though, just something that it happened to expose.

[rcorrie]

@richardgirges thanks but I still get the same error

[mattmcdonald-uk]

@richardgirges Following those exact steps, same error 😣

[fmaceachen]

Default metro-bundler: 0.13.0 and "moment": "2.19.0" that just released fixed the issue for me.

Regards.

[richardgirges]

I'm reproducing this as well:

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

[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.

[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.

[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.

[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

[cpojer]

You can probably already do require.call(null, 'Foo') but then 'Foo' might not exist in the final bundle, especially as we replace IDs. I think @davidaurelio was pointing out to me today that we need to track other metadata, but let's start with the simple PR first :)

[jrylan]

[Removed] (I was experiencing a different problem than the issue posted, but receiving the same error.)

[tqc]

@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.

[sjmueller]

The things I've tried so far to get our app working again:

• Take dependency on tqc/moment#no-dynamic-import, blows up with moment.locale undefined
• Fork @tqc's fork to fix more spots blowing up, still failing
• Uncomment the line that throws the error, it just blows up later
• Add return statement before that line, the bundle finally compiles but then back to moment.locale undefined
• Force react native to use version 0.18 of metro-bundler instead of 0.19, blows up with TypeError: Right-hand side of 'instanceof' is not an object

All out of options, what can I do now?

[TikiTDO]

@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.

[fungilation]

All out of options, what can I do now?

@sjmueller I downgrade RN back to 0.48.4 by reverting git changes, nuke /node_modules and run yarn again. Is that cheating?

[tqc]

@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?

[TikiTDO]

@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 import "moment/locale/en-ca" in an initializer can be a valid solution; it's not a matter of that call fixing partial imports as I believed earlier, but it's simply the fact that calling this import already does all the work for you. Afterwards the loadLocale function can safely fail without affecting operation.

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 rn-cli.config.js file to set the extraModules parameter, but again that's pretty work intensive, and it's something that needs to live in the root app, making it troublesome to configure in a lib.

[tqc]

@TikiTDO That line is exactly where the problem is - it resolves to an instance of moment, but not the same one imported from the root app.

It’s pretty easy for a module to include both es5 and es6 entry points, but I’m not aware of any equivalent for multiple files, and the wide variety of platforms supported by moment makes platform detection within the file problematic.

Do you have a pointer to documentation on that config file setting? A config in the root project might be ok, since at that point you can know that a module is trying to load something incompatible.

[TikiTDO]

@tqc Ah, I see what you mean. You set the react-native endpoint to src/moment-core.js, which instantiates a brand new instance of moment, but importing "moment/locales/whatever" does a relative require which imports the main entry point.

I don't think there's a trivial way to use custom entry points, alias locale path, and not break metro-bundler. Any tool-based solution I can think of would be a pretty big non-starter for a project like moment.

One potential generic solution may be to modify the files in the moment/locales directory to require moment wholesale (potentially with a relative fallback). In this situation metro-bundler would just transform that require to import the react-native endpoint by number, which should then register the locale with the correct instance.

Alternatively, you could just extend the functionality of the moment-core.js loadLocale to simply reference all possible imports explicitly (or implement a locale index.js, which could be auto-generated), and select the correct one to execute with a switch statement. This comes with the downside that all the locales would be included in every bundle, but the upside that the app devs wouldn't need any hacks to get the locales working.

The rn-cli.config.js is read just once, when the react-native command is started, so having a copy in a module will not help any. If you're interested the best documentation I've found is in the source here, though I had to trace through in a debugger to really figure out what half of this stuff actually does. Using a babel plugin is also likely to be a no-go, given that moment doesn't seem to use babel.

[tqc]

@TikiTDO Once I get off a plane I’m planning to push a branch that changes locale files, see what happens with that. The switch in there is the best I’ve got so far, but it might still pull in two copies of moment, one unused.

Including all locales would have its advantages, but not great for web projects, and other modules dont necessarily use the moment locale loader.

I was thinking more along the lines of adding a doc on he moment side saying how to get it working on rn, but I see what you mean about it being too much work for the average user.

[TikiTDO]

@tqc How about something like this:

// src/locale/index.js

export af from "af"
export arDz from "ar-dz"
export arKw from "ar-kw"
export arLy from "ar-ly"
// This file can be auto-generated
//...

// moment-core.js

loadLocale = (name) => {
  if (locales[name]) {
    const camelName = magicCamelize(name)
    const locales = require("./locale/")
    locales[camelName]
  }
  //...
}

With that, any project that uses a bundled script will include all the locales in the bundle. However, there's really not any way around that if you want to have said locales available. In this situation it's possible to modify the locales so they don't actually become memory resident until they are requested.

For any project that offers a full ES6 environment nothing has to change. If a system has access to a proper require implementation then it can load files as necessary. Otherwise it's quite reasonable to expect most web projects and the like will continue to use the ES5 entry point, which doesn't have to have any new behavior at all.

[tqc]

@TikiTDO That would work, albeit at the cost of a much larger bundle. However, getting a locale into memory is the easy bit - the tricky part is when you have a module that imports a locale file - at that point you don't have control over what code is getting imported.

[TikiTDO]

@tqc Once the locale data is in memory it doesn't really matter if someone imports a locale file (Though they really shouldn't be important locales at all in that case). The wrong import may instantiate another momemnt instance, and waste more memory, but it wouldn't really break anything.

At that point it becomes a question of knowing what an import does, and this is a problem that extends to more than just moment. If you're importing things without whether you need to import them, then you're not going to have control of what code gets imported anyway.

[RichardMcSorley]

@tqc Any updates to this issue? We ran into this issue just 30 minutes ago... 😉

[eggybot]

any update to this issue? Got this error

error: bundling failed: Error: require() must have a single string literal argument
    at pushDependency (/Users/me/mobileapps/react/chatapp2/node_modules/react-native/node_modules/metro-bundler/src/JSTransformer/worker/extract-dependencies.js:39:13)
    at CallExpression (/Users/me/mobileapps/react/chatapp2/node_modules/react-native/node_modules/metro-bundler/src/JSTransformer/worker/extract-dependencies.js:50:9)
    at NodePath._call (/Users/me/mobileapps/react/chatapp2/node_modules/babel-traverse/lib/path/context.js:76:18)
    at NodePath.call (/Users/me/mobileapps/react/chatapp2/node_modules/babel-traverse/lib/path/context.js:48:17)
    at NodePath.visit (/Users/me/mobileapps/react/chatapp2/node_modules/babel-traverse/lib/path/context.js:105:12)
    at TraversalContext.visitQueue (/Users/me/mobileapps/react/chatapp2/node_modules/babel-traverse/lib/context.js:150:16)
    at TraversalContext.visitMultiple (/Users/me/mobileapps/react/chatapp2/node_modules/babel-traverse/lib/context.js:103:17)
    at TraversalContext.visit (/Users/me/mobileapps/react/chatapp2/node_modules/babel-traverse/lib/context.js:190:19)
    at Function.traverse.node (/Users/me/mobileapps/react/chatapp2/node_modules/babel-traverse/lib/index.js:114:17)

[albinekb]

So, if we had a hook into where this error gets thrown, we could handle it (manually import the correct locales here for example). Could that be an acceptable solution? Kinda like we can hook into webpack config.

[fungilation]

For sure I/we want to get to the bottom of this. Right now the assumption is cause being something broken in Bundler as that's what the error on CLI bundling tells us, but it could be something else.

The optics is that me and others, most with Moment dependency, see our RN apps break with the bundling error on RN 0.49.1. And I've never seen this bundling error before since RN 0.3x. The crux is like you said, we don't really know the exact cause or how to fix this. Only that something between RN 0.48 and 0.49 broke.

[richardgirges]

@cpojer agreed. I apologize if I'm stirring up a ruckus - not my intention at all. I completely understand and respect the fact that OSS is always a side hustle for the maintainers who have a full time gig and a life outside of OSS.

You bring up an interesting point. If metro bundler has never supported dynamic imports, I wonder if the issue we're seeing is simply that metro bundler has just recently begun enforcing static imports? A quick git blame appears to imply that the throw which enforces static requires is only two months old: https://github.com/facebook/metro-bundler/blame/66182916de8e5bb0e55b637a3587532f55a9cf12/packages/metro-bundler/src/JSTransformer/worker/extract-dependencies.js#L44

If I'm reading that correctly, then perhaps the recent enforcing of static requires is simply uncovering the fact that metro bundler never supported them?

[richardgirges]

Nice!! Sounds like we cracked the case :D

Look forward feedback on the PR during the week. Cheers 🥂

[fungilation]

Facebook pays us to work on these projects because they are useful.

And indeed, React and RN are pioneering work that's moving the industries (Web and Mobile) forward. Their "usefulness" is indeed what sets Facebook apart from other tech giants, as I know it is something more battle tested than most closed source software will ever know, and the best of breed in design thought and community support.

This is also why it's frustrating when things keep breaking. Please don't take offence when people complain, including myself. Entitlement and tone are obviously off putting, but more importantly I think is constructive criticism. If something like this issue breaks so many projects, it does need more priority in addressing. And perhaps architectural thought on why it (Metro Bundler) keeps breaking, or perhaps that's redundant as it's already said Facebook have much work planned on cleaning up Bundler for the better. Applause for that.

OSS is great as it's beneficial to all parties. Constructive criticism and cooperation moves us all forward, and appreciation to contributors goes without saying.

[cpojer]

If you'd like to help improve how RN and Metro are integrated, this is a good start: facebook/react-native#15945

It's most likely something we won't do, but would help both RN and Metro.

[TikiTDO]

We make mistakes. It is impossible not to at this scale. With hundreds of thousands of dependants, something somewhere is going to break at some point. At the same time we juggle hundreds of other issues internally. It is frustrating if your code doesn’t work after upgrading. However, at the same time, due to today’s bugs, it doesn’t work on master for some teams at Facebook. If we don’t prioritize those problems, nobody at Facebook will want to use React Native, and the project will die. Obviously that would be even worse for the community longer term. So we have to balance the urgency of issues affecting FB and the community, and it is hard. We’re trying our best but I’m curious to know what strategy you would employ if you were in our shoes.

@gaearon You hit upon my main issue in here. I can understand delays, I can understand that there may be styles and guidelines inherent in contributing to a project that someone that isn't a common contributor might not understand, I can understand that software is a pursuit that takes time, dedication, and patience, often when those qualities are hard to come by. As much as many of those things annoy me, I've been writing software long enough to be affected by, and be responsible for each of those dozens of times over. What does truly confuse me is the level of community attention a project like this merits in comparison to the more shiny and interesting projects like react, react-native, and even your own redux.

A bundler affects to operations of libraries that stretch the entire range of JavaScript world. It's the point of interface between react-native and everyone. A major bug here has the potential to completely break an huge number of installations, particularly when combined with a semi-forced react-native update (RN 0.49 was necessary to fix Oreo development, which was a major issue for me). In the 11 days you are the second FB employee to participate in this thread, and while I can respect and appreciate the work @cpojer has done, it's been clear from our interaction that he alone does not have the decision making authority to quickly resolve something like this.

You asked what the rest of the people here would do in your shoes; to start with I would ensure that there exists an escalation path to quickly involve a few more developers in issues that clearly have wider scope than minor bug reports and small feature requests. Looking at other heavily discussed issues on other facebook projects, as soon as an issue blows up there are either already a bunch of facebook people involved from the start, or someone people will tend to cc a few more developers to join the discussion.

Hearing feedback along the lines of "I'll take it up with the team and get back to you next week" is quite disheartening when it would take the team in question five minutes to all chime in. Granted, that might not resolve the question any quicker, but it would at least give the impression that an issue is being taken seriously. Again, this is only something I'd consider necessary for only the most major of issues, but it is an escalation route that should exist, and should be communicated to members of the team.

[cpojer]

@TikiTDO I think your perception of this is quite wrong. As I outlined above, making the wrong decision here could lead to missing dependencies in production which could lead to significant problems for us. We also use a different bundling system at Facebook (the second part of Metro that is not yet open source) that is incompatible with dynamic requires for various reasons. Even though the team working on Metro reports to me, I'm sure you can appreciate that I would not make such a decision without talking to the engineers working on this project even if I can (and for that matter, any other person working at Facebook can, we do not have a concept of authority or code ownership that you speak of). Also, it's a weekend and I'm not really sure what the point of ongoing discussion here is as I promised we'll take care of it next week. @richardgirges also pointed out multiple ways to unblock yourself in the meantime. I would like to either ask you to wait until we publish a fix or spend your energy helping us make this project better for everyone, which I explained is on top of our mind. If you (or anyone else reading this) are serious about helping out, please email me at [email protected] and I'll help you get started.

[TikiTDO]

@cpojer I beg pardon, but it feels like you did not read my previous comment.

The idea I presented in my last post is that there should be an escalation path to involve more developers in the resolution of major issues, rather than less. This is explicitly because making the wrong decision could cause a problem. Most of the people here can not walk to another facebook engineer's desk to discuss an issue, and are not privy to the content of such discussions. My question is, if this is a discussion that affects an open source project, why can't it happen on an open forum, with participation from more of the affected developers and stakeholders?

Note: At this point the actual issue in the title has been resolved for me in three different ways. I assure you the organizations I consult for are no longer affected by it, and my primary development workload has moved well past this issue.

My goal here, the only reason I'm still participating, is to ensure such an issue is handled better the next time it happens. This is why I am taking time out of my own weekend, hammering out what I believe to be detailed explanation. There are clearly organizational shortcomings that can be improved upon, which is a matter completely separate from dynamic requires. If such shortcomings can be mitigated by adding a line to an issue reading cc @insert_name_here then I think that's a discussion worth everyone's time.

[vovkasm]

I think that #69 is a good hot fix, and hope that metro bundler team will apply it asap.

As for the future, can metro-bundler support some sort of configurable registry of modules to pack to the bundle, that can be handled by dynamic require? Then dynamic require can inject module if it packed, or throw runtime error, if module was not packed.

[sjmueller]

I really appreciate the prompt fix @cpojer and others who helped.

Hopefully the perspective I expressed here does not get misconstrued. This was much less about the singular issue, but rather an ongoing disconnect that seems to be repeatedly breaking the consumers of RN over several months. In a sense, this particular regression may have been the straw that broke the camel's back for me and others.

That being said, I took the time to re-read the entire thread, and after fully understanding your position and contributions @cpojer, it's really not fair you receive the brunt of these frustrations. React native has a disconnect that is much more of a systemic problem, but this was not the right forum to talk about those problems in a constructive manner.

[rcorrie]

@romreed

Latest release of react-native is depending on a certain version of metro-bundler that is causing this error.

At some point today, metro-bundler team will tag a new version of their package.

Once that happens, with Yarn, you will be able to selectively override the version required by your react-native library.

Hope this helps, I know how confusing things can get in the beginning.

[jeanlauliac]

Okay! Can you paste the full stacktrace, if available?

[rcorrie]

@jeanlauliac
I could probable give more meaningful information if I could open up the remote debugger, but when I do the app instantly crashes.

[richardgirges]

@rcorrie @fmaceachen I'm not running into that issue. Not sure if this could be related but here is how I added the new metro-bundler release to my package.json:

  "dependencies": {
    "react": "16.0.0-beta.5",
    "react-native": "0.49.3"
  },
  "resolutions": {
    "react-native/metro-bundler": "0.20.0"
  },

[mattmcdonald-uk]

Using the resolutions suggested by @richardgirges, I'm still receiving the error Uncaught Reference Error: process is not defined.

[richardgirges]

@rcorrie @mattmcdonald-uk can you post up your version of RN (down to the patch number) along with your dependencies? Might help to find a pattern so that I can reproduce it on my end.

[mattmcdonald-uk]

  "dependencies": {
    "@expo/ex-navigation": "3.0.0",
    "axios": "0.16.2",
    "babel-preset-react-native-stage-0": "1.0.1",
    "expo": "17.0.0",
    "moment": "2.18.1",
    "pusher-js": "4.1.0",
    "react": "16.0.0-beta.5",
    "react-native": "0.49.3",
    "react-native-fabric-twitterkit": "0.2.0",
    "react-native-fbsdk": "^0.6.1",
    "react-native-image-picker": "0.26.7",
    "react-native-keyboard-aware-scroll-view": "0.3.0",
    "react-native-linear-gradient": "^2.3.0",
    "react-native-maps": "0.16.1",
    "react-native-permissions": "1.0.1",
    "react-redux": "5.0.6",
    "react-string-replace": "0.4.0",
    "redux": "3.6.0",
    "redux-persist": "4.8.3",
    "redux-thunk": "2.2.0",
    "uuid": "3.0.1"
  },
  "resolutions": {
    "react-native/metro-bundler": "0.20.0"
  }

[rcorrie]

I am using the same versions you suggested. This is kinda weird though, what does your npm ls metro-bundler say @richardgirges ?

[email protected] /Users/rcorrie/Workspace/--redacted--
├── [email protected]  extraneous
└─┬ [email protected]
  └── UNMET DEPENDENCY metro-bundler@^0.13.0

npm ERR! extraneous: [email protected] /Users/rcorrie/Workspace/--redacted--/node_modules/metro-bundler
npm ERR! missing: metro-bundler@^0.13.0, required by [email protected]

Seems like the version resolution might not be working well in my case.

[mattmcdonald-uk]

@rcorrie If your issue is also moment related, the workaround they just released has fixed this issue for me (using [email protected])

https://github.com/moment/moment/blob/9483e2a49fd8479fb84f1e00d6a854f106abc950/moment.js#L1846

[richardgirges]

Interesting. Okay so I was able to reproduce the issue in the midst of moving the metro-bundler dependency in and out of the resolutions property in package.json. Unclear exactly how that came to be about. But in the end, I deleted the yarn.lock file, re-ran yarn install (using the resolutions solution I posted above), and it worked.

@rcorrie here is my npm ls metro-bundler output:

[email protected] /repo/trueflip/sandbox/code/trueflipapp
└── (empty)

[richardgirges]

Here are the exact steps I took:

1. rm yarn.lock
2. Kill any currently open packager process
3. watchman watch-del-all
4. Add the following to package.json: "resolutions": { "react-native/metro-bundler": "0.20.0" }
5. yarn install

[rcorrie]

@richardgirges @mattmcdonald-uk

I get the same error still.

[rcorrie]

What @fmaceachen suggested works for me as well, thanks!

[kelset]

Hey @fmaceachen sorry to bother you, what do you mean with using default metro-bundler: 0.13.0? That you don't have it explicitly installed?
Can you please share your package.json?

[mattmcdonald-uk]

@kelset If you haven't changed your metro-bundler dependency then no change would be needed, just update moment.

[rcorrie]

@mattmcdonald-uk is everything working for you after @fmaceachen 's suggestion?

[kelset]

Ok thanks @mattmcdonald-uk for the clarification, then my guess is that all the libs that have moment.js installed need to update to the newer moment version... right?

[mattmcdonald-uk]

@rcorrie Yep.

[kelset]

Thanks @richardgirges, gonna try it asap!

[fmaceachen]

All is working, with new moment: 2.19.0 and yes I removed the metro-bundler resolution.
0.13.0 is the one that RN depends on.

[jasdeepsingh]

Whats the suggested/minimum version of react we should be using with 0.20.0?

[richardgirges]

Aha! I found a solid repro for the metro-bundler issue after blowing out my node_modules directory. It appears to have surfaced as early as metro-bundler 0.19.0 (possibly even earlier). Chances are that the RN team will catch it and prioritize it before the next major release of RN goes out. I'll go ahead and make a new ticket to track this issue.

For the time being, I believe pointing to the hot fixed version of moment and using yarn resolutions resolve the new version of moment as sub-dependencies should be sufficient.

@jasdeepsingh it appears to master branch in RN is pointing to 16.0.0 so I would guess this version of metro would support that.

[kelset]

@richardgirges your solution to "override" the dependency in the lib doesn't seem to be working :(

I get this warning: warning Resolution field "[email protected]" is incompatible with requested version "[email protected]" and it still uses 2.18.

[richardgirges]

I've created #73 to track the "process variable" error.

@kelset this is what I was afraid of. Pointing to the updated moment is a viable solution for anyone who doesn't have a sub-dependency of moment.

What package specifically are you trying to override moment.js for?

[kelset]

react-native-gifted-chat; since that workaround didn't work I created a PR to the lib with the updated dependency to moment 2.19.0 and I'm going to use my forked patch until merge.

---

by using my patched version now I don't get the error anymore from the lib, but now I have it directly from my project 😰 yes, I've updated moment 2.19, removed node modules, cleaned watchman, etc etc and it's still there :/

Any clue of what I may be missing?

[clementdevosmc]

Version 0.20.0 fixes the issue for me.
Cheers.

[rcorrie]

@kelset
Step 1: Open up node_modules/react-native-gifted-chat/package.json and make sure the version number is the one you are trying to override it with.

Step 2: If the version is not the proper one, it might be because your version of yarn does not have the resolutions feature. Make sure you update your version of yarn to the latest (1.2.0).

Then run yarn and redo Step 1

[kelset]

@rcorrie thanks for the hint, probably I haven't expressed myself well - I don't have any resolutions in my package.json because it was giving me back a warning. So I forked the lib, patched it by updating the dependency to moment 2.19.0 and then having my package.json to point to it.
by doing so that library now doesn't give me the require error anymore. But now the require error says that it's the moment lib in node_modules to "fail"; and this doesn't make much sense to me since my package.json has moment 2.19.0 as dependency.

Is it more clean this way? I'm on mobile ATM so not sure.

[rcorrie]

@kelset

Got it, I wasn't caught up on your issue. Are you properly clearing everything out when you test your changes? To be sure, run through these steps:

rm yarn.lock
watchman watch-del-all
rm -rf node_modules
yarn
rm -fr $TMPDIR/react-*

[maggiepint]

This is an extremely long thread, and I am about to sink some time into trying to figure out what is going on, but I will be reverting the fix in moment soon, as the change we made broke all webpack users. Is there any guidance as to other courses of action we can take besides renaming require in Moment?

[richardgirges]

@maggiepint long story short, there was a new fix merged in for metro-bundler (#69) and a new release was cut (v0.20.0).

However, now there's another blocker issue in metro-bundler: #73. If #73 gets fixed, the patch in moment will no longer be needed. This new blocker issue is unrelated to this issue entirely and my guess is that the metro team will address it after they get up to speed.

For clarity, you're talking about reverting the fix in moment, correct?