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

There is a problem with the configuration. #196

Closed
wants to merge 2 commits into from
Closed

There is a problem with the configuration. #196

wants to merge 2 commits into from

Conversation

fro9-dev
Copy link

No description provided.

@@ -26,11 +26,11 @@ const gitInfo = () => {
*/
const writePackage = async (template, { user, email }) => {
let pkg = {}
const name = baseName(process.cwd())
const source = `src/${name}.${template}`
let name = baseName(process.cwd())
Copy link
Author

Choose a reason for hiding this comment

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

Is there a better way to keep const?

Copy link
Owner

Choose a reason for hiding this comment

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

how about split on path.delimiter?

https://nodejs.org/api/path.html#path_path_delimiter

Copy link
Author

@fro9-dev fro9-dev Mar 10, 2021

Choose a reason for hiding this comment

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

Is it possible to process / and \ separately?

And I think we need to resolve the overwrite of the name in pkg.name.

Copy link
Owner

Choose a reason for hiding this comment

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

The overwrite is already handled in merge right after.

The path separator is platform specific if you use path.delimiter linked above.

Copy link
Author

@fro9-dev fro9-dev Mar 11, 2021

Choose a reason for hiding this comment

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

Even after merge
You need to fix the part where the name value is kept as the system folder name.

Copy link
Author

Choose a reason for hiding this comment

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

It is set as const name in all the codes below.
However, if pkg.name exists
name must be replaced with pkg.name

Is my opinion correct?

Copy link
Author

Choose a reason for hiding this comment

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

	const name = process.cwd().split('/').pop()
	const source = `src/${name}.${template}`

	if (exists('./package.json')) {
		pkg = JSON.parse(await read('./package.json'))
	}

	pkg = merge(
		{
			name,
			version: '0.0.0',
			license: 'MIT',
			description: name,
			type: 'module',
			exports: {
				'.': {
					browser: `./dist/${name}.js`,
					import: `./dist/${name}.js`,
					require: `./dist/${name}.cjs.js`,
					umd: `./dist/${name}.umd.js`,
				},
				'./': './',
				'./package.json': './package.json',
			},
			keywords: [name],
		},
		pkg
	)

yes pkg merged.
But
name is replaced with
const name = process.cwd().split('/').pop()

Is this what you intended?

Copy link
Owner

Choose a reason for hiding this comment

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

oh my bad, I did not see the subsequent usage.

Copy link
Owner

Choose a reason for hiding this comment

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

and now that I see it, I think there are more cases where this will break.

Copy link
Author

Choose a reason for hiding this comment

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

I'll go to your house and try!

thx!

@osdevisnot
Copy link
Owner

👋 @fro9-dev thank you for the catch.

osdevisnot added a commit that referenced this pull request Mar 11, 2021
the package name is derived from current working directory and operations on directory paths should
use correct separator based on platform

re #196
@osdevisnot
Copy link
Owner

@fro9-dev can you try the latest version 6.1.1 on windows and see if you still see an issue.

@fro9-dev
Copy link
Author

All my problems have been solved.

I'll close this PR.

Thank you for your quick feedback 👋

@fro9-dev fro9-dev closed this Mar 11, 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.

2 participants