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

Adds version from package.json to the default CLI message #144

Merged
merged 4 commits into from Nov 4, 2021

Conversation

peschee
Copy link
Contributor

@peschee peschee commented Oct 21, 2021

It would be nice if the default CLI message would show the installed version of the analyser.

I have added parsing the version from the package's package.json file.

@netlify
Copy link

netlify bot commented Oct 21, 2021

✔️ Deploy Preview for custom-elements-manifest-analyzer ready!

🔨 Explore the source changes: 55982fc

🔍 Inspect the deploy log: https://app.netlify.com/sites/custom-elements-manifest-analyzer/deploys/61725a1f2023c8000769a015

😎 Browse the preview: https://deploy-preview-144--custom-elements-manifest-analyzer.netlify.app

@@ -120,8 +120,8 @@ export function addCustomElementsPropertyToPackageJson(outdir) {
}
}

export const MENU = `
@custom-elements-manifest/analyzer
export const MENU = ({ version = ''}) => `
Copy link
Member

Choose a reason for hiding this comment

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

instead of refactoring this to a function, we can just read the package.json here:

const {version} = JSON.parse(`${fs.readFileSync(path.resolve('package.json'), 'utf-8')}`);
export const MENU = `@custom-elements-manifest/analyzer ${version}` // etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep reading the package.json in index.js since it is probably easier to get the path right. I'm not sure if we can use __dirname here to get to the package file from src/utils

What's your take?

Copy link
Member

Choose a reason for hiding this comment

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

ah yes, good point. The way its currently implemented will resolve the package.json of the package the analyzer is used in, not the analyzer's package.json as well.

const packageJsonPath = new URL('../../package.json',  import.meta.url).pathname;
const { version } = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8'));

^ That should work, but I'm not sure if that works on windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

../../ won't work on Windows, so we'd have to use path.resolve('..', '..', 'package.json') but that should work.

I think import.meta won't be available in all node environments, though. What versions should the analyser support?

Copy link
Member

Choose a reason for hiding this comment

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

Node 14>

Copy link
Member

Choose a reason for hiding this comment

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

This works:

import { createRequire } from 'module';
const require = createRequire(import.meta.url);
const { version } = require('../../package.json');

export const MENU = `
@custom-elements-manifest/analyzer ${version}` // etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏼

@thepassle thepassle merged commit 656c708 into open-wc:master Nov 4, 2021
@thepassle
Copy link
Member

Thanks for your contribution!

@thepassle
Copy link
Member

Published in v0.5.7

@peschee peschee deleted the feature/cli-version branch November 4, 2021 08:43
@peschee
Copy link
Contributor Author

peschee commented Nov 4, 2021

👍🎉

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.

None yet

2 participants