Skip to content

Add node resolution for transformer #940

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

Merged
merged 6 commits into from
Nov 16, 2019

Conversation

willemneal
Copy link
Contributor

Try relative path first and then with normal node resolution.

@MaxGraey
Copy link
Member

MaxGraey commented Nov 7, 2019

How about use require.resolve and check returned string instead try / catch wrapper over require?

@jtenner
Copy link
Contributor

jtenner commented Nov 7, 2019

@MaxGraey is require.resolve() something that AssemblyScript is going to support at some point?

@MaxGraey
Copy link
Member

MaxGraey commented Nov 7, 2019

This actually node (commonjs) mechanics. Don't think we need this in current time until we load everything ahead of time

@willemneal
Copy link
Contributor Author

@MaxGraey To use resolve we would still have to check for an error:

> require.resolve("a")
Thrown:
Error: Cannot find module 'a'
Require stack:
- <repl>
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:625:15)
    at Function.resolve (internal/modules/cjs/helpers.js:21:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '<repl>' ]
}

@MaxGraey
Copy link
Member

MaxGraey commented Nov 7, 2019

@willemneal Yes, but you got only MODULE_NOT_FOUND error and don't need _require. With wrapping whole module you should check error type because failing could be in different reasons

@MaxGraey
Copy link
Member

MaxGraey commented Nov 7, 2019

function tryResolvePath(path) {
  try { return require.resolve(path) }
  catch (e) { return null }
}

@willemneal
Copy link
Contributor Author

willemneal commented Nov 7, 2019

So something like this:

function resolve(m) {
  try {
    return require.resolve(m);
  } catch (e) {
    return null;
  }
}

and

const classOrModule = (resolve(filename) == null) ? require(transformArgs[i]) : require(filename);

@willemneal
Copy link
Contributor Author

Just tested it with my transformer and it works.

@MaxGraey
Copy link
Member

MaxGraey commented Nov 7, 2019

great

cli/asc.js Outdated
@@ -220,7 +229,7 @@ exports.main = function main(argv, options, callback) {
: path.join(process.cwd(), filename);
if (/\.ts$/.test(filename)) require("ts-node").register({ transpileOnly: true, skipProject: true });
try {
const classOrModule = require(filename);
const classOrModule = require(pathResolves(filename) ? filename : transformArgs[i]);
Copy link
Member

@dcodeIO dcodeIO Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, transfromArgs[i] is not trimmed, which filename is guaranteed to be, and trying to require that is unnecessary if the path is absolute anyway. Also, require(transformArgs[i]) is relative to node_modules/assemblyscript/cli/asc.js which appears problematic. A cleaner variant might be

  • if absolute, require as-is
  • if relative, use node-resolve relative to cwd? (not sure about --baseDir)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve can take an array of paths to use instead of the default, so we could use cwd and baseDir there.

Also why is the filename not guaranteed to be trimmed? Shouldn't all cli arguments be trimmed?

@willemneal
Copy link
Contributor Author

willemneal commented Nov 7, 2019

Now we have

function _require(m, paths) {
  const _path = require.resolve(m, {paths});
  return require(_path)
}
//and

let filename = transformArgs[i].trim();
if (/\.ts$/.test(filename)) require("ts-node").register({ transpileOnly: true, skipProject: true });
try {
   const classOrModule = _require(filename, [baseDir, process.cwd()]);

If the path is absolute that it doesn't matter for the resolution paths, otherwise you try to look starting in both baseDir and process.cwd().

@willemneal
Copy link
Contributor Author

Okay testing shows that this resolves currently in both situations where asc is locally in "node_modules" or from a different location, e.g. globally. However, when asc is non-local I get the following:

ERROR: TypeError: Cannot redefine property: signbit
    at Function.defineProperties (<anonymous>)
    at Object.<anonymous> (/Users/willem/near/near-assemblyscript/near-bindgen-as/tests/node_modules/assemblyscript/dist/webpack:/assemblyscript/std/portable/index.js:280:8)
    at Object.call (/Users/willem/near/near-assemblyscript/near-bindgen-as/tests/node_modules/assemblyscript/dist/assemblyscript.js:1:592833)
    at t (/Users/willem/near/near-assemblyscript/near-bindgen-as/tests/node_modules/assemblyscript/dist/webpack:/assemblyscript/webpack/bootstrap:19:22)
    at Object.call (/Users/willem/near/near-assemblyscript/near-bindgen-as/tests/node_modules/assemblyscript/dist/webpack:/assemblyscript/src/glue/js/index.ts:10:1)
    at t (/Users/willem/near/near-assemblyscript/near-bindgen-as/tests/node_modules/assemblyscript/dist/webpack:/assemblyscript/webpack/bootstrap:19:22)
    at Object.call (/Users/willem/near/near-assemblyscript/near-bindgen-as/tests/node_modules/assemblyscript/dist/assemblyscript.js:1:586627)
    at __webpack_require__ (/Users/willem/near/near-assemblyscript/near-bindgen-as/tests/node_modules/assemblyscript/dist/webpack:/assemblyscript/webpack/bootstrap:19:22)
    at /Users/willem/near/near-assemblyscript/near-bindgen-as/tests/node_modules/assemblyscript/dist/webpack:/assemblyscript/webpack/bootstrap:83:10
    at /Users/willem/near/near-assemblyscript/near-bindgen-as/tests/node_modules/assemblyscript/dist/assemblyscript.js:1:124

This is because my transformer requires "assemblyscript" and asc requires "../dist/assemblyscript". So when when everything is local this ends up being the same file and assemblyscript is not loaded the second time.

One solution could be to pass the compiler and then the transformer adds the types file to its config. Alternatively we can just keep the requirement that you must keep them together.

@willemneal
Copy link
Contributor Author

I've tested this and it works for my transformer, but again only if they are both local.

@willemneal
Copy link
Contributor Author

@dcodeIO @MaxGraey Any more thoughts about this? I think it is a fair assumption that if you are using the transformer, to install the transformer class along side the compiler.

@dcodeIO dcodeIO merged commit aa296f6 into AssemblyScript:master Nov 16, 2019
@dcodeIO
Copy link
Member

dcodeIO commented Nov 16, 2019

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants