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

Renames near.gitignore to .gitignore during project creation #67

Merged
merged 7 commits into from
Feb 15, 2020

Conversation

mikedotexe
Copy link
Contributor

Note: I'd like some guidance with folks familiar with publishing this to npm. I'm not aware of how to "test" this yet without publishing.

From my understanding (see links below) the .gitignore files we have created are not being created when using the generator commands like npx create-near-app….
This is not good as end users may accidentally commit sensitive info, like in the neardev directory.

For context, here's two links discussing the missing .gitignore files in folks' generators.
npm/npm#1862
npm/npm#3763

Reading through those, there's discussion of adding a .npmignore file, but I don't think those threads are on track with our issue here.

My solution here is to add a new file, near.gitignore that will not be omitted during the process of publishing to npm. Then in the package.json files under the scripts key adding postinstall and simply renaming it after the npm install. (rename near.gitignore to .gitignore)

Then when it comes to our own development, we'd like a normal .gitignore file, so I've created a symlink (.gitignore » near.gitignore) so we don't have to maintain two of those files.


While I couldn't test the npx command, I did locally remove the node_modules directory and npm i and see that this works, and that git uses the symlink like I hoped it would.

@mikedotexe
Copy link
Contributor Author

This person proposes the idea of "whitelisting" files. Something to think about:
https://medium.com/@jdxcode/for-the-love-of-god-dont-use-npmignore-f93c08909d8d

@vgrichina
Copy link
Contributor

BTW, for now I've just changed .gitignore files to be more specific, because they also cause test keys to not be included (breaking running tests)
#69

@mikedotexe
Copy link
Contributor Author

BTW, for now I've just changed .gitignore files to be more specific, because they also cause test keys to not be included (breaking running tests)
#69

Interesting. So perhaps the .gitignore file we want the user to have after the npx generator command is going to be different than our own internal .gitignore
For instance we won't want this removed from the end user's file, right?
https://github.com/nearprotocol/create-near-app/pull/69/files#diff-67d2838502483fecce1727e6ebd976e3L2

@icerove
Copy link
Contributor

icerove commented Jan 24, 2020

do we still need this?

@mikedotexe
Copy link
Contributor Author

do we still need this?

I personally think it's pretty important to ship a default .gitignore file to the end user.
It will be hard for them to know that they need to add neardev themselves, for instance. And if they push up code to a public repo, that wouldn't be great.
This morning we also talked about moving all the keys to ~/.near/[some-directory], which would help.
I'd be happy to chat about this with folks.

@behaviary behaviary self-requested a review January 24, 2020 02:24
@mikedotexe
Copy link
Contributor Author

So this is the most important doc I found about this:
https://docs.npmjs.com/misc/developers#keeping-files-out-of-your-package

If we want someone to use the generator command(s) npx create-near-app path and end up with a healthy .gitignore file, I think the only way is this postinstall option in package.json

OR

We should whitelist the files as described there.
One cool thing is you can try playing around with .gitignore and .npmignore and then run:

npm pack

This will create a tarball that can be extracted and you'll see exactly what you'd get after running the generator command.

@behaviary behaviary requested a review from vgrichina February 3, 2020 14:29
@@ -26,7 +26,8 @@
"deploy:pages": "gh-pages -d build",
"deploy": "npm run deploy:contract && npm run deploy:pages",
"build": "react-scripts build && gulp",
"dev:deploy:contract": "near dev-deploy ./out/main.wasm"
"dev:deploy:contract": "near dev-deploy ./out/main.wasm",
"postinstall": "mv near.gitignore .gitignore"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how this should work. Like this is a script for template project, my understanding is that it won't be executed during project generation. It would be executed when someone installs generated project though. But we want .gitignore at time of project generation.

@vgrichina
Copy link
Contributor

@mikedotexe I think the easiest solution is just name file differently in repo (e.g. gitignore without dot) and then rename it when copying files to create new project (somewhere here https://github.com/nearprotocol/create-near-app/blob/master/index.js#L28)

Symlinks won't work on Windows I think and aren't very critical as generally there shouldn't be keys created inside of templates folders when developing this project.

@mikedotexe
Copy link
Contributor Author

@mikedotexe I think the easiest solution is just name file differently in repo (e.g. gitignore without dot) and then rename it when copying files to create new project (somewhere here https://github.com/nearprotocol/create-near-app/blob/master/index.js#L28)

Symlinks won't work on Windows I think and aren't very critical as generally there shouldn't be keys created inside of templates folders when developing this project.

I like this approach. Thanks for pointing me to that function, @vgrichina. I just pushed up that change. This PR is ready for a re-review

index.js Outdated
@@ -61,6 +61,20 @@ const create_Project = async function(options) {
}
});
});
// rename proper (non-excluded by npm) default gitignore file to .gitignore
const gitignoreFile = `${projectDir}/near.gitignore`;
fs.access(gitignoreFile, fs.constants.F_OK | fs.constants.W_OK, (err, second) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's best to use promise-based API in async function
https://nodejs.org/dist/latest-v10.x/docs/api/fs.html#fs_fs_promises_api

current implementation doesn't wait properly for operations to complete before logging console.log('Copying project files complete.');

@vgrichina
Copy link
Contributor

@mikedotexe please rebase on master and make sure to update Rust template as well.

@mikedotexe mikedotexe force-pushed the postinstall-rename-gitignore-#66 branch from 7d92285 to 827885d Compare February 6, 2020 16:50
@mikedotexe mikedotexe changed the title Proposal: use symlinks for .gitignore files, postinstall renames the target file back Renames near.gitignore to .gitignore during project creation Feb 6, 2020
@mikedotexe
Copy link
Contributor Author

@amgando noticed this issue here as well. #99
Bump on this topic.
Travis is acting up and that's why it looks like it's failed:
FetchError: request to http://shared-test.nearprotocol.com:3030/ failed, reason: socket hang up

@behaviary
Copy link

This is good to go if tests are not passing simply due to flakiness as mentioned. I would re-run trying to make sure that's the case, then merge away.

@vgrichina vgrichina merged commit 896b023 into master Feb 15, 2020
@mikedotexe mikedotexe linked an issue May 26, 2020 that may be closed by this pull request
@volovyks volovyks deleted the postinstall-rename-gitignore-#66 branch May 5, 2021 15:19
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.

.gitignore file never created
4 participants