-
-
Notifications
You must be signed in to change notification settings - Fork 337
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
Remove dist/ files from git #2463
base: develop
Are you sure you want to change the base?
Conversation
Also, the |
It was mentioned/suggested on Discord that generating beta docs would be a good idea (which IMO would happen when there are updates to either this repo for nightly or the docs repo for nightly) - I would have thought updating the |
We will keep the /dist folder. Especially if we would update it via the nightly script and create a nightly beta tag. So, the /dist folder would be present / recreated every night and that would mean to also remove the dist folder again in a separate commit , which is a mess of git history changes. Not to commit files inside the/dist folder is mentioned in the contributing.md already Line 47 in 0740de8
If you would like to help out for that, you may try to adjust the nightly.js script instead
Cannot reproduce. Using |
This is something i would kind of agree with (instead of deleting the /dist folder) but the above mentioned nightly script should be adjusted accordingly nevertheless |
b4022d1
to
ac870d8
Compare
ac870d8
to
d49f362
Compare
d49f362
to
f42138f
Compare
16f3ba4
to
009b20e
Compare
Fixed. The |
Your PR still deletes the /dist folder which i told you we won't do. Instead what you wanted is to have the dist folder being updated by the nightly script in the git repo. But this is not what your changes do. Your changes just make sure that the npm package includes the generated dist folder (which is fine) and that /dist is ignored for commits (which is also fine) but it actually does not update the git repo itself. |
I know, but why? What is the purpose of having stable/nightly I have described the reason (in updated) PR description, I think we should really not have these files in develop branch, local rebuild should really not imply additional git tracked changes files locally. |
|
What about introducing a
|
Your main concern was that you accidently had /dist updated files in another PR some weeks ago, where i asked you to remove them. Why don't we start with that first? 🙂 |
I’d stick with just a |
96be289
to
a475786
Compare
It may sounds so, but the main reason is: compile should not change any git (non-gitignored) file. This is industry standard practice. Also there is no rational reason, as done currently, to store stable/compiled release together with develop changes. This PR removes the With this reasoning I belive this PR can be merged safely. |
Currently,
dist/
files are comming from the latest stable release and storing them in versioned git is useless and it complicates development. Local changes followed by a local rebuild should not imply additional (git tracked) changed files,This PR removes
dist/
directory from git and adds.gitignore
exclude pattern for it. In release scripts (nightly/stable), the exclude pattern is removed sodist/
files are pushed to npm as before.