Skip to content
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

Allow version tags in aliases #2480

Merged
merged 6 commits into from
Jun 24, 2019

Conversation

Hypercubed
Copy link
Contributor

Here is PR for allowing version tags in aliases for your review.

Note: I had a hard time compiling this for testing. I was seeing many TS errors that I needed to hack around. What is your build process? Can we add that to the package scripts?

@Hypercubed
Copy link
Contributor Author

Here are my TS fixes: https://github.com/Hypercubed/jspm-cli/tree/fix/ts-issues

Not sure if you need these.

Copy link
Member

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Amazing! Please add yourself to CONTRIBUTORS if not already - https://github.com/jspm/jspm-cli/blob/master/CONTRIBUTORS.

Also the TypeScript fixes would be great to include either in this PR or another. I usually run the build with vscode so that's why that folder is checked in.

src/cli.ts Outdated Show resolved Hide resolved
src/utils/common.ts Outdated Show resolved Hide resolved
install.name = install.target.name.substr(install.target.name.lastIndexOf(':') + 1);
const idx = install.target.name.lastIndexOf(':') + 1;
const substr = install.target.name.substr(idx);
if (substr.match(validAlaisRegEx))
Copy link
Member

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 this part since install.target is supposed to be the registry:name@version thing right? So in jspm install @x=x, the install.target should be the npm:x part right? Not the @x part? It's been a while since I've looked at this code, so maybe you can clarify this for me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not really necessary for this PR. This was a refactor to reduce some redundant code while I was debugging.

@Hypercubed
Copy link
Contributor Author

@guybedford Did you see the changes here: https://github.com/Hypercubed/jspm-cli/tree/fix/ts-issues? Not sure if or how this was working previously.... but I need that to run the tests.

@Hypercubed
Copy link
Contributor Author

I updated this PR including the changes I need to get TS to run.

@guybedford guybedford merged commit 9a46037 into jspm:master Jun 24, 2019
@guybedford
Copy link
Member

Published in 2.0.0-beta.5. Thanks so much for the ts build fixes too. Usually we don't commit the build changes, and only do that for release, but it was fine here.

@jbanety
Copy link
Contributor

jbanety commented Jun 26, 2019

Hi,
This PR introduces some bugs.

Command :
jspm build $(jspm trace --deps ./public/src/app.js) -h --dir ./public/dist -o ./public/dist/deps-buildmap.json

Error raised :

err (jspm) TypeError: Function has non-object prototype 'undefined' in instanceof check   
    at Function.[Symbol.hasInstance] (<anonymous>)    
    at isRecord (/jspm/lib/build/index.js:40:18)   
    at Object.build (/jspm/lib/build/index.js:43:9)   
    at cliHandler (/jspm/lib/cli.js:602:44)   
    at Object.<anonymous> (/jspm/lib/cli.js:699:5)   
    at Module._compile (internal/modules/cjs/loader.js:776:30)   
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:787:10)   
    at Module.load (internal/modules/cjs/loader.js:653:32)   
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)     
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)

@guybedford
Copy link
Member

@jbanety thanks for reporting - I believe this should be fixed in #2482.

@guybedford
Copy link
Member

Released in 2.0.0-beta.6.

@Hypercubed
Copy link
Contributor Author

Oof... that was a half completed change. 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.

3 participants