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

Error: Can't resolve ./src/adb #16

Closed
NoahAndrews opened this issue Jul 23, 2020 · 7 comments · Fixed by #18
Closed

Error: Can't resolve ./src/adb #16

NoahAndrews opened this issue Jul 23, 2020 · 7 comments · Fixed by #18
Labels
bug Something isn't working

Comments

@NoahAndrews
Copy link

NoahAndrews commented Jul 23, 2020

When I try to use adbkit in an Electron project I'm working on, I encounter the following errors:

 ERROR in C:/Users/Noah/.../node_modules/@devicefarmer/adbkit/index.js
[1]   Module not found: Error: Can't resolve './src/adb' in 'C:\Users\Noah\...\node_modules\@devicefarmer\adbkit'
[1]    @ C:/Users/Noah/.../node_modules/@devicefarmer/adbkit/index.js 9:15-35
[1]    @ ../test-plugin/dist/index.js
[1]    @ ./src/main.ts
[1]    @ multi C:/Users/Noah/.../node_modules/electron-webpack/out/electron-main-hmr/main-hmr ./src/main.ts
[1]
[1]   ERROR in C:/Users/Noah/.../node_modules/@devicefarmer/adbkit-logcat/index.js
[1]   Module not found: Error: Can't resolve './src/logcat' in 'C:\Users\Noah\...\node_modules\@devicefarmer\adbkit-logcat'
[1]    @ C:/Users/Noah/.../node_modules/@devicefarmer/adbkit-logcat/index.js 9:15-38
[1]    @ C:/Users/Noah/.../node_modules/@devicefarmer/adbkit/lib/adb/client.js
[1]    @ C:/Users/Noah/.../node_modules/@devicefarmer/adbkit/lib/adb.js
[1]    @ C:/Users/Noah/.../node_modules/@devicefarmer/adbkit/index.js
[1]    @ ../test-plugin/dist/index.js
[1]    @ ./src/main.ts
[1]    @ multi C:/Users/Noah/.../node_modules/electron-webpack/out/electron-main-hmr/main-hmr ./src/main.ts
[1]
[1]   ERROR in C:/Users/Noah/.../node_modules/@devicefarmer/adbkit-monkey/index.js
[1]   Module not found: Error: Can't resolve './src/monkey' in 'C:\Users\Noah\...\node_modules\@devicefarmer\adbkit-monkey'
[1]    @ C:/Users/Noah/.../node_modules/@devicefarmer/adbkit-monkey/index.js 9:15-38
[1]    @ C:/Users/Noah/.../node_modules/@devicefarmer/adbkit/lib/adb/client.js
[1]    @ C:/Users/Noah/.../node_modules/@devicefarmer/adbkit/lib/adb.js
[1]    @ C:/Users/Noah/.../node_modules/@devicefarmer/adbkit/index.js
[1]    @ ../test-plugin/dist/index.js
[1]    @ ./src/main.ts
[1]    @ multi C:/Users/Noah/.../node_modules/electron-webpack/out/electron-main-hmr/main-hmr ./src/main.ts

I'm using electron-webpack to build the binary, but adbkit is included by way of a yarn workspace dependency (I'm using yarn 2 with the node-modules nodeLinker option).

I can't figure out why I'm having the issue, and I can't reproduce it with a clean electron-webpack project. Rather than focusing on what's causing the issue, it might be easier to try to find a resolution. For example, is it really necessary to cater to coffeescript users at this point? Could we just remove the check and always return the js version?

@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label bug to this issue, with a confidence of 0.74. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@issue-label-bot issue-label-bot bot added the bug Something isn't working label Jul 23, 2020
@koral--
Copy link
Member

koral-- commented Jul 23, 2020

Could we just remove the check and always return the js version?

Which check do you mean?

@NoahAndrews
Copy link
Author

Sorry for the ambiguity, I was referring to this check (and its counterparts in adbkit-monkey and adbkit-logcat):

adbkit/index.coffee

Lines 4 to 5 in f7c57a4

when '.coffee' then require './src/adb'
else require './lib/adb'

@koral--
Copy link
Member

koral-- commented Jul 27, 2020

Removing coffeescript does make sense. However, I'm not sure if it is so simple as removing 1 condition.
Here is the issue which may be relevant: openstf#48

@NoahAndrews
Copy link
Author

Yeah, I'm familiar with that issue, that's why I was comfortable saying "is it really necessary to cater to CoffeeScript users at this point?".

That issue is about converting the project completely away from CoffeeScript, which would be nice, but it's definitely a big task, and I don't think it's necessary for what I'm proposing.

That switch statement in index.coffee/index.js checks if it's being executed as a CoffeeScript file, or as a transpiled JavaScript file. I don't think we need to do that. In fact, I don't think there's a single case where that file gets executed as a CoffeeScript file. The project's tests depend directly on the relevant file in the src folder, and the src files don't even get distributed by npm, so unless I'm missing something, even consumers of this library who use CoffeeScript are actually using the version that's been transpiled into JavaScript. If that's the case, then it's definitely 100% safe to remove the switch statement, and to always call require(lib/adb).

For further evidence, here's another actively maintained library written in CoffeeScript: https://github.com/Leonidas-from-XIV/node-xml2js

They don't have any checks for the .coffee extension in their codebase. Instead, they treat the CoffeeScript files in the src folder only as the source code, and the JavaScript files in the lib folder as the only output. In their package.json's main and directories entries, they directly reference the lib folder.

Finally, here's an excerpt from the CoffeeScript V1 website:

The golden rule of CoffeeScript is: “It’s just JavaScript”. The code compiles one-to-one into the equivalent JS, and there is no interpretation at runtime. You can use any existing JavaScript library seamlessly from CoffeeScript (and vice-versa).

Everything I'm seeing seems to indicate that you don't have to do anything special to support consumers of your library that happen to be written in CoffeeScript. They should use the transpiled JavaScript, just like everybody else.

@NoahAndrews
Copy link
Author

NoahAndrews commented Jul 28, 2020

Since I'm extra confident about the safety of my proposal after additional research, I submitted PR #18.

@NoahAndrews
Copy link
Author

Thanks, now I need to do the same thing for adbkit-monkey, and then change this library to depend on the latest versions of adbkit-monkey and adbkit-logcat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants