-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
Add support of non '.js' main #1291
Conversation
Thanks. Did you manage to take a look at the failing test? |
Looking at it. |
It actually fail on main. It's not related to this PR. I checkout jspm/master and get same error. |
Really weird. Saw they pass on travis history, but I hard reset local to 0.16.14 (e5e9f27) and run test it still fail with same error. |
@unional that would be due to caching. Run |
Done! |
if (match) { | ||
mainFileExtension = match[1]; | ||
main = main.slice(0, - mainFileExtension.length); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow why this is necessary? Surely line 783 can be left as-is with the same effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is checking for the lastNamePart
. Could it has non .js
there? If it can't, then this is redundant.
Coverage test failed with: non-issue? |
return new Promise(function(resolve) { | ||
mainPath = path.resolve(downloadDir, main + '.js'); | ||
mainPath = path.resolve(downloadDir, main + mainFileExtension); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be '.js'
.
This looks good, thanks for getting it to this point! Could you do a squash of these commits with a force push to clear up the history? Otherwise I can do it on merge no problem too. |
c4979a3
to
e52bb2a
Compare
Like this? |
Looks like it caught some other stuff there, perhaps it needed a rebase as well? |
Seems like the same error as the coverage failure last time. Is there a way to retry the build without committing? |
I'm not so worried about that, but see the extra file contents at https://github.com/jspm/jspm-cli/pull/1291/files. |
Oh....how come... |
e52bb2a
to
7089f78
Compare
I did revert+amend to fix it. Seems good now. Is that the best way to do it? I still have lots of stuff to learn in using git. And thanks for the careful code reviews. :) |
Add support of non '.js' main
👍 |
One custom step need is to add
packages: { "myModule": { defaultExtension: "{myExtension, e.g. ts}" } }
, which is ok to do before 0.17