Skip to content

Conversation

@dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Sep 16, 2019

This PR refactors path resolving in asc by moving the logic into its own function. There's also a new API now to obtain the source text in source map post-processing that fixes #831.

cc @willemneal - does this look good to you?

@willemneal
Copy link
Contributor

Yeah that looks good. @jtenner are still working on an external resolver (#796) so this is good first step for that too.

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 16, 2019

That code is still quite hard to grasp, so I tried to clean it up even more. While necessary imo I do hope that I don't break it. Also added a little something for @org/package resolving, but the test I added is relatively basic.

@willemneal
Copy link
Contributor

Looking at the test, having it depend on something else is a key part, and I think it's a fair assumption that the root of the project there is a package.json file, even if it doesn't use the ascMain field.

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 17, 2019

Turned out that getPaths was doing crazy stuff, like concatenating two absolute paths, and something leading to the d test finding the top-most as module I think it shouldn't find. Still not quite sure what was going on there, and why, but renaming the source folder d/node_modules/as/as to d/node_modules/as/assembly makes it find that dependency. Though I'm wondering if something of this was intended? If so, what was being tested there?

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 17, 2019

One remaining problem seems to be that there can't be multiple modules of the same name, depended upon by different sub-modules, due to naming everything ~lib/packageName.

@willemneal
Copy link
Contributor

Turned out that getPaths was doing crazy stuff, like concatenating two absolute paths, and something leading to the d test finding the top-most as module I think it shouldn't find. Still not quite sure what was going on there, and why, but renaming the source folder d/node_modules/as/as to d/node_modules/as/assembly makes it find that dependency. Though I'm wondering if something of this was intended? If so, what was being tested there?

It was testing if it could find an different location for a entry. In as/package.json there should be a field like "ascMain": "./as/index.ts".

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 17, 2019

Thanks, so instead of renaming as/as -> as/assembly I went with renaming as/as -> as/notassembly and added an as/package.json with ascMain = ./notassembly/index.ts.

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 17, 2019

Also figured that having multiple packages of the same name will require some sort of support by the compiler itself, since it only requests ~lib/packageName, no matter the import site, and expects to get that back exactly, so we can't just make a ~pkg/:a:b/... scheme on the asc side without the compiler knowing about it. Suggesting to leave this for a future PR.

@willemneal
Copy link
Contributor

Yeah that is what I've found. I also think in that PR we should add compiler errors that return the real location of the file. This is handy for several reasons, but currently I find it useful because I can do (ctrl/Cmd) + click on the file name and it will open it to the line of the file in my IDE.

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.

Wrong paths for node modules with cli --sourceMap option

3 participants