Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
TikiTDO opened this issue Sep 28, 2017 · 166 comments

Comments

Projects
None yet
@TikiTDO
Copy link

commented Sep 28, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2017

I'm reproducing this as well:

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

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2017

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

This comment has been minimized.

Copy link

commented Oct 1, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2017

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

@cpojer cpojer closed this Oct 1, 2017

@TikiTDO

This comment has been minimized.

Copy link
Author

commented Oct 4, 2017

@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

This comment has been minimized.

Copy link

commented Oct 4, 2017

@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

This comment has been minimized.

Copy link
Author

commented Oct 4, 2017

@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

This comment has been minimized.

Copy link

commented Oct 4, 2017

@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

This comment has been minimized.

Copy link
Author

commented Oct 4, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

@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 comment has been minimized.

Copy link

commented Oct 4, 2017

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 cpojer reopened this Oct 4, 2017

@TikiTDO

This comment has been minimized.

Copy link
Author

commented Oct 4, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

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 :)

@fungilation

This comment has been minimized.

Copy link

commented Oct 5, 2017

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

@jayrylan

This comment has been minimized.

Copy link

commented Oct 5, 2017

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

@sjmueller

This comment has been minimized.

Copy link

commented Oct 5, 2017

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.

@fungilation

This comment has been minimized.

Copy link

commented Oct 5, 2017

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

This comment has been minimized.

Copy link

commented Oct 5, 2017

@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

This comment has been minimized.

Copy link

commented Oct 5, 2017

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

This comment has been minimized.

Copy link
Author

commented Oct 5, 2017

@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

This comment has been minimized.

Copy link

commented Oct 6, 2017

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

This comment has been minimized.

Copy link

commented Oct 6, 2017

@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

This comment has been minimized.

Copy link
Author

commented Oct 6, 2017

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

@jeanlauliac

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2017

I think this issue is back again because we switched Metro itself to use the new module collectDependencies, that strictly enforces static requires, and no automated test was covering this use case, so we did not realise. Two action items:

  • Write a test case for this in the integration test (basic_bundle).
  • Allow dynamic requires based on a whitelist. In an initial implementation, just throw an error when these requires are invoked in production (that's why it requires a whitelist).

I'll look into this today.

Eventually, we could add support for dynamic requires, though some more thoughts will be needed (we translate file paths to numeric IDs, so it's not that straightforward to support dynamic module paths/names).

@krishnalelas

This comment has been minimized.

Copy link

commented Dec 7, 2017

Working on RN 0.51.0. Finally! 👍
"react-native": "0.51.0",
"metro-bundler": "0.20.3",
"moment": "2.19.2"

@rush86999

This comment has been minimized.

Copy link

commented Dec 25, 2017

this problem is back I don't know a workaround to this.

{
  "main": "node_modules/expo/AppEntry.js",
  "private": true,
  "scripts": {
    "test": "node ./node_modules/jest/bin/jest.js --watch"
  },
  "jest": {
    "preset": "jest-expo"
  },
  "dependencies": {
    "@expo/samples": "2.1.1",
    "@expo/vector-icons": "^6.2.2",
    "axios": "^0.17.1",
    "expo": "^24.0.0",
    "firebase": "^4.8.1",
    "libphonenumber-js": "^0.4.48",
    "lodash": "^4.17.4",
    "moment": "^2.20.1",
    "qs": "^6.5.1",
    "react": "16.0.0",
    "react-native": "https://github.com/expo/react-native/archive/sdk-24.0.0.tar.gz",
    "react-native-elements": "^0.18.5",
    "react-native-image-gallery": "^2.1.4",
    "react-native-modalbox": "^1.4.2",
    "react-navigation": "^1.0.0-beta.22",
    "react-redux": "^5.0.6",
    "redux": "^3.7.2",
    "redux-persist": "^5.4.0",
    "redux-thunk": "^2.2.0"
  },
  "devDependencies": {
    "babel-eslint": "^7.2.3",
    "eslint": "^4.14.0",
    "eslint-config-airbnb": "^16.1.0",
    "eslint-plugin-import": "^2.8.0",
    "eslint-plugin-jsx-a11y": "^6.0.3",
    "eslint-plugin-react": "^7.5.1",
    "jest-expo": "^23.0.1",
    "reactotron-react-native": "^1.14.0",
    "reactotron-redux": "^1.13.0"
  },
  "resolutions": {
    "moment": "2.20.1"
  }
}

facebook-github-bot added a commit that referenced this issue Dec 28, 2017

collectDependencies: provide more data in invalid-require error
Summary: This improves the error message by providing line/column so we can identify errors encountered in #65 easier. Also provide additional fields so we can better format that later on.

Reviewed By: cpojer

Differential Revision: D6641909

fbshipit-source-id: 0372da6b2fb51010b42ab906fc40b528b1483532
@NichAga

This comment has been minimized.

Copy link

commented Jan 9, 2018

Yes the problem is back with react-native 0.52.0 and momentjs 2.20.1
Not happening with the same momentjs version on react-native 0.51.0

EDIT:
Solution -> just don't use any .min file (of moment). Use "moment/min/moment-with-locales.js"

@jeanlauliac

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2018

Solution -> just don't use any .min file. Use "moment/min/moment-with-locales.js"

Note the bundler for React Native will do minification anyway when you generate a production bundle.

@NichAga

This comment has been minimized.

Copy link

commented Jan 9, 2018

@jeanlauliac

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.
Also updated my comment with the specification : (of moment).

@jeanlauliac

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

I realised a possible solution for moment.js would be to leverage the react-native field in the package.json. Imagine you have a file dynamicDeps.js that contains the following line:

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 dynamicDeps.js that doesn't have a non-static require and provide a reasonable alternative for React Native instead. We can use the react-native field of the package.json:

{
  "name": "some-package",
  ...,
  "react-native": {
    "./lib/dynamicDeps.js": "./lib/dynamicDeps_forRN.js",
  }
}

Then, dynamicDeps.js can stay the same, and dynamicDeps_forRN.js would look like:

function getSmth(name) {
  throw new NotAvailableError('Smths are not available with React Native');
};

Thanks to the react-native field, Node.js would pick up the original file, while RN would only pick up the special file, that doesn't contain dynamic requires, and as such doesn't cause Metro to bail out.

@vovkasm

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

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:

  1. stops using numeric ids for internal modules index and use module path strings as other packagers do... then support for dynamic require will be as trivial as in other packagers
  2. maintain meta-info to make translation of module path to internal module id on runtime (may be optional, may be configurable per project)

Current metro behavior simple incorrect...

@TikiTDO

This comment has been minimized.

Copy link
Author

commented Jan 11, 2018

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

@vovkasm

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

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

@TikiTDO

This comment has been minimized.

Copy link
Author

commented Jan 12, 2018

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

@jeanlauliac

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2018

Metro should not force library autors to some special support.

I'd like to nuance this a little bit: there are lots of JavaScript packages on npm that cannot be run on the web out-of-the-box, for example. Notably, if they depend on Node.js libraries. There are ways to work around that, like browserify providing polyfills for some of these libraries, but this is a specific choice that was made by the bundler. Another of these workarounds was to provide the browser field, that allows you to provide alternative implementations for the web. So these kind of differences aren't new, there's no canonical way to run JavaScript. Not supporting dynamic require calls in Metro is a choice that was made in a particular context some time ago, and it hasn't changed not because one didn't want to, mostly because one would have to put work into it. See also earlier in this thread, where the tradeoffs were extensively discussed.

So what I was trying to say is, lightweight workarounds for metro/react-native are either using the react-native field, or using a custom transform that strips dynamic requires before the dependency collection happens.

@fossage

This comment has been minimized.

Copy link

commented Jan 16, 2018

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.

@jeanlauliac

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2018

Alright, mitigation on the way. We'll make it so dynamic requires from within node_modules are transformed so as to throw at runtime, rather than at compile-time. This should allows using moment.js and other modules with no modification necessary to these modules.

facebook-github-bot added a commit to facebook/react-native that referenced this issue Jan 18, 2018

metro: allow dynamic dependencies from within node_modules
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

facebook-github-bot added a commit that referenced this issue Jan 18, 2018

metro: allow dynamic dependencies from within node_modules
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
@alexstoyanov

This comment has been minimized.

Copy link

commented Jan 22, 2018

@jeanlauliac That is great! I tried the latest metro code from master and I am getting an compile time error:
/alexander/Development/shkolo-mobileapp/node_modules/metro/src/babelRegisterOnly.js:27
);
^
SyntaxError: Unexpected token )
at createScript (vm.js:56:10)
at Object.runInThisContext (vm.js:97:10)
at Module._compile (module.js:542:28)
at Object.Module._extensions..js (module.js:579:10)
at Module.load (module.js:487:32)
at tryModuleLoad (module.js:446:12)
at Function.Module._load (module.js:438:3)
at Module.require (module.js:497:17)
at require (internal/module.js:20:19)
at Object. (/alexander/Development/shkolo-mobileapp/node_modules/react-native/setupBabel.js:12:27)
Any help would be appreciated!

@jeanlauliac

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

I think you cannot use master directly, because it's not precompiled and setup properly. Can you try metro@0.24.6, perhaps? It should contain the related change.

@alexstoyanov

This comment has been minimized.

Copy link

commented Jan 22, 2018

@jeanlauliac Yes, it is working with metro@0.24.6. Thank you!

@theronic

This comment has been minimized.

Copy link

commented Feb 3, 2018

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.

@jeanlauliac

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

Howdy! Normally this should be mitigated in the last version of Metro: packages in node_modules should be able to require() modules dynamically and still be able to be transformed and bundled. These requires won't work at runtime however (it will throw when the dynamic require is made), but most packages have these requires optional to start with.

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!

@vyshkant

This comment has been minimized.

Copy link

commented Mar 5, 2018

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

@jeanlauliac

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

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?

grabbou pushed a commit to react-native-community/cli that referenced this issue Sep 26, 2018

metro: allow dynamic dependencies from within node_modules
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.