Add node module resolution by default and use --path for custom package locations

[willemneal]

By default uses a method similar to node-reslove to find packages. A package either has an assembly folder or an ascMain field in a package.json.

If your project root is located at /home/willem/c/hello, then the valid paths from the following are added:

• /home/willem/c/hello/node_modules
• /home/willem/c/node_modules
• /home/willem/node_modules
• /home/node_modules
• /node_modules

--path relative/path adds a path to the list of default paths.

The big difference between this method and node is that because node dynamically loads the modules, imports start their search from the location of the file being loaded. However, currently a parsed files doesn't track the location of the file that imported it. So instead when a package is found its path is joined with node_module or relative/path which found it.

For example, if a package was found at .../hello/node_modules/b, then ..../hello/node_modules/b/node_modules is added to the list of paths.

[willemneal]

Now it looks for an ascMain field in the packages package.json and defaults to assembly/index.ts.

Also not sure why it's failing since the log on Travis says it passed.

I'm going to add my tests from another branch soon.

[willemneal]

@dcodeIO @MaxGraey I've now got tests for testing the new argument and added two new commands:
--traceResolution: prints out the steps taken to resolve packaged files.
--listFiles: prints out the list of files to be compiled and exits.

You can also run the tests with npm run test:packages, which installs a set of interdependent packages with lerna and then runs tests on them using a version of as-pect that depends on this version of asc.

[MaxGraey]

I don't think so using as-pect or any other third-party testing library is good idea

[dcodeIO]

Should go to stderr - stdout might be module output

[willemneal]

Okay will change.

[willemneal]

I guess that should also be the case for traceResolution.

[dcodeIO]

In general I think that the debugging might help now to implement the feature, but I'm skeptical that it deserves actual compiler options that nobody will ever use, unless debugging the feature.

[willemneal]

Well typescript has the option and I did need a way to debug it. I'm not attached though. tsc also has --all which is where you'll find these types of commands not just --help.

[dcodeIO]

Hmm, hiding these behind an -all flag sounds good, yeah.

[willemneal]

Should I add that now or do a future PR?

[dcodeIO]

Also not sure why it's failing since the log on Travis says it passed.

That's because dist files have been touched (see).

[willemneal]

@MaxGraey Why? as-pect is a project that is intimately linked to the compiler and stays up to date with the master branch. Plus it provides a clean way of maintaining tests whereas handwritten ones do not. Clearly we can't use it to test the core compiler, but for behavior like this it seems very suitable. If there is a change that does break as-pect then that would end up breaking a lot of projects that depend on as-pect. @jtenner and I have spent a great deal of time working and testing it out and will continue to ensure that it's a dependable tool for development assemblyscript with.

I understand your hesitation on adding this dependency, but from personal experience as-pect has been the best way to be productive in AssemblyScript. Especially since it removes the need to setup a loader and imports. Not to mention it's very fast.

[willemneal]

Also not sure why it's failing since the log on Travis says it passed.

That's because dist files have been touched (see).

Yeah I figured that out from the logs and revert them.

[dcodeIO]

I agree with Max that adding additional dependencies (this isn't about as-pect) just to test this feature is overkill. The list of dependencies is intentionally minimal so package size doesn't tank unexpectedly or builds get broken by something external. Would prefer to keep it that way.

[MaxGraey]

@willemneal as-pect produce circular dependency which could cause to problems when AS significantly change. Without circular dependency it will overkill because as-pest will require (and download) other version of assemblyscript. So all this looks weird and unnatural

[willemneal]

Well it's only a dependency of the subpackage and so it will only be installed for testing. I understand your hesitation, just wish there was a better way to add tests. I suppose I could use WASI, but node doesn't support it yet and then you'd depend on another runtime. My last point I want to make again is that if as-pect is broken by a PR, that's bad news for all those who use it.

I'll go ahead and remove the lerna dependency as it's not really needed now that I can use my node script to build and run the tests. And if you really insist I'll remove as-pect as well.

[MaxGraey]

No as-pect is great tools. But it depends on AssemblyScript. And some bugs which could be already fixed in AS could still present in as-pest like this one. It may happen that we get "dependency racing" (like data racing)

[dcodeIO]

LGTM with the remaining comments addressed :)

[dcodeIO]

Hmm, strange, now seeing this error during npm run test:packages:

> @ b XY\tests\packages
> cd ./packages/b && node ../../../../bin/asc assembly/index.ts --noEmit --runtime stub && node ../../../../bin/asc assembly/index.ts --noEmit --runtime stub --listFiles

ERROR: Import file '~lib/a.ts' not found.

[willemneal]

Weird. It still passes on my end and passed Travis.

[dcodeIO]

With --traceResolution included in the b script the output is

Looking for ~lib/a imported by XY\tests\packages\packages\b
    in node_modules
    in ..\..\..\..\node_modules
    in node_modules
ERROR: Import file '~lib/a.ts' not found.
    at parseBacklog (XY\cli\asc.js:442:25)

Any idea?

[willemneal]

I'm assume this is Windows? I'm not sure what XY is.

[dcodeIO]

XY is just a substitute for my local path where the assemblyscript repo is living. And yes, Windows.

[dcodeIO]

Somewhere, the path to a file becomes ..\..\..\..\node_modules~lib/a, in turn resulting in a plainName of node_modules~lib/a. Looks like it's confusing / and \.

[willemneal]

Ah okay, well try it again. I just moved the printing of where it's looking, so that we can get a better idea of what's going on. E.g. now it prints this

Looking for ~lib/c imported by /Users/willem/Research/wasm/assemblyscript/tests/packages/packages/d
    in node_modules/assembly/c
Found at node_modules/c/assembly/index.ts

[willemneal]

Hrmm, well I think it's because of adding SEP. Perhaps we should just stick with / as I believe that node handles the conversion when using / pathnames in windows. Considering that everywhere before it was assumed to have /. Could you try making SEP = "/" and see if that fixes it?

[dcodeIO]

I think the use of SEP is fine, but it's currently also used for internal paths it seems, which are always using /. When assigning realPath for example, the _p input is an internal path it seems, but the splitting is performed with SEP.

[dcodeIO]

This works:

          let realPath = (_p) => {
            if (_p.startsWith(exports.libraryPrefix)){
              _p = _p.substring(exports.libraryPrefix.length);
            }
            let first = _p.substring(0, _p.indexOf("/"));
            let second = _p.substring(_p.indexOf("/") + 1);
            return path.join(_path, first, ascMain, second);
          }

[willemneal]

Great can you commit that change? I think you have permission. I'm afk.

[willemneal]

nvm just got back to my computer.

[dcodeIO]

Great, thanks, working now :)