search

vert-x3 / vertx-lang-js

Nashorn JavaScript implementation for Vert.x
Apache License 2.0
35 stars 23 forks source link

Implementation of NODE style lookup is incomplete #61

Closed DanielJoyce closed 5 years ago

DanielJoyce commented 8 years ago

node_module/a/b/c.js node_module/a/lib/index.js

c.js:

require('../lib')
// MOAR CODE...

these kinds of relative imports fail because jvm-npm.js fails to check for ../lib/index.js which is a valid possibility for node style file resolution

Patching it right now on my end

DanielJoyce commented 8 years ago

I suspect its the resolveAsClasspath(Node)Module methods

DanielJoyce commented 8 years ago

Havent had time to look at it but it definitely messes up deployment of fat jars.

pmlopes commented 8 years ago

There has been a update to the original jvm-npm.js that we should either try to synchronize or review the whole js loader.

cescoffier commented 8 years ago

We need to come up with more tests here. I've seen the new jvm-npm.js but before updating we would need a better test suite (and being sure to test these on Linux and Windows).

DanielJoyce commented 8 years ago

Just from analyzing the docs on node module resolution and looking through the code, it seems to be a very messy implementation.

Also although it seems like standard Node/Javascript development practice, it looks for node_module 'roots' in multiple places, which is a bomb waiting to happen when deployed code stops working because it depended on a module in user.dir/node_modules which doesn't exist in production and the module wasnt tracked in a package.json.

I think roots should be very explicitly set beyond say the cwd.

pmlopes commented 6 years ago

@DanielJoyce some work has been done on a totally new JavaScript support and the require issues have been fixed there: https://github.com/pmlopes/vertx-lang-es

Backporting is not trivial has the way the current generated code works expects that those bugs are present and writing workarounds would make it even more complex than it is right now.

Please note that this new code is still in development and not on npm yet but feel free to look and comment.

pmlopes commented 5 years ago

This issue cannot be fixed without a use breakage so the advice is to upgrade to https://reactiverse.io/es4x/