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

Remove dist/ files from git #2463

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Sep 25, 2022

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 so dist/ files are pushed to npm as before.

@mvorisek
Copy link
Contributor Author

mvorisek commented Sep 25, 2022

When building the dist/ data using https://fomantic-ui.com/introduction/getting-started.html#installing-via-npm instructions, semantic.css is not built (only the minified file is built).

I would expect build result 1:1 same with the official release, ie. incl. semantic.css.

Also, the node_modules/fomantic-ui/package.json version is "version": "2.8.8". When I require "fomantic-ui": "^2.9.0-beta.319" nightly package, I would expect that version everywhere. Nightly tags should be build exactly the same as the stable releases are. Develop branch version should be something like 2.9.0-dev.

@jamessampford
Copy link
Contributor

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 dist/ dir would also be a good idea as part of the nightly, seeing that the beta docs would need the updated files, as well as everything else being up to date

@lubber-de
Copy link
Member

lubber-de commented Sep 25, 2022

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

- When you commit don't include your dist files, this can cause merge conflicts and also adds unnecessary changes to the PR.

If you would like to help out for that, you may try to adjust the nightly.js script instead
I did not research yet, but maybe we can setup some bot which will remove any changes from the /dist folder from any incoming PR (if it's not meant for a release merge..)

When building the dist/ data using fomantic-ui.com/introduction/getting-started.html#installing-via-npm instructions, semantic.css is not built (only the minified file is built).

Cannot reproduce. Using npx gulp build or yarn build creates both files semantic.css/js and semantic.min.css/js for me

@lubber-de lubber-de added state/wont-fix Anything which isn't going to be fixed state/on-hold Issues and pull requests which are on hold for any reason labels Sep 25, 2022
@lubber-de
Copy link
Member

When the dist/ content is built for a (stable & nightly) release, the /dist/ line in .gitignore should be removed and files added to git.

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

@mvorisek mvorisek force-pushed the rm_dist_from_develop branch from b4022d1 to ac870d8 Compare November 3, 2022 10:10
@mvorisek mvorisek force-pushed the rm_dist_from_develop branch from ac870d8 to d49f362 Compare November 10, 2022 22:19
@mvorisek mvorisek force-pushed the rm_dist_from_develop branch from d49f362 to f42138f Compare December 9, 2022 23:37
@mvorisek mvorisek marked this pull request as draft December 9, 2022 23:45
@mvorisek mvorisek force-pushed the rm_dist_from_develop branch 2 times, most recently from 16f3ba4 to 009b20e Compare December 9, 2022 23:53
@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 9, 2022

When the dist/ content is built for a (stable & nightly) release, the /dist/ line in .gitignore should be removed and files added to git.

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

Fixed. The /dist/ line in .gitignore is removed before the package is pushed to the registry, so the built dist/ is pushed to npm registry correctly.

@mvorisek mvorisek marked this pull request as ready for review December 9, 2022 23:58
@mvorisek mvorisek changed the title Remove dist/ from develop Remove dist/ from git Dec 9, 2022
@mvorisek mvorisek changed the title Remove dist/ from git Remove dist/ files from git Dec 10, 2022
@lubber-de
Copy link
Member

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.

@mvorisek
Copy link
Contributor Author

Your PR still deletes the /dist folder which i told you we won't do.

I know, but why? What is the purpose of having stable/nightly dist/ files in develop branch (and in git in general)?

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.

@lubber-de
Copy link
Member

  1. We are following gitflow principle, so the master branch will always contain the same content as the develop branch (at point of merge), because master is always a merge of develop and should not have dedicated commits (which would be needed if only the master branch should contain /dist).
  2. There are still a lot of people not using cdn/npm or do/can not build locally but just want to download the repo as it is ready for usage. Those people also want to have the /dist folder ready for usage without the need to compile themselves. Yes, that's exactly the point where we would need improvement for the develop branch, as the /dist inside the develop branch currently does not reflect the changes in /src as you initially pointed out.

@mvorisek
Copy link
Contributor Author

What about introducing a nightly branch?

  • in develop, there will be no dist/ (thus a local rebuild will not imply additional (git tracked) changed files), point I solve by this PR now
  • in nightly there will be merged in develop + unignored dist/ files rebuilt by CI, point you want to address
  • in master the same but stable release instead of nightly release, point we support currently by updating dist/ once per stable release

@lubber-de
Copy link
Member

lubber-de commented Dec 10, 2022

Your main concern was that you accidently had /dist updated files in another PR some weeks ago, where i asked you to remove them.
You solved that situation in this PR here by adding /dist to .gitignore but also making sure that the nightly/release script temporary removes it from gitignore right before publishing.

Why don't we start with that first? 🙂

@jamessampford
Copy link
Contributor

jamessampford commented Dec 10, 2022

What about introducing a nightly branch?

  • in develop, there will be no dist/ (thus a local rebuild will not imply additional (git tracked) changed files), point I solve by this PR now
  • in nightly there will be merged in develop + unignored dist/ files rebuilt by CI, point you want to address
  • in master the same but stable release instead of nightly release, point we support currently by updating dist/ once per stable release

I’d stick with just a develop and master, where develop is always updated, even the dist/ folder, keeps everything consistent

@lubber-de lubber-de removed the request for review from exoego January 8, 2023 14:42
@mvorisek mvorisek force-pushed the rm_dist_from_develop branch from 96be289 to a475786 Compare January 20, 2023 18:41
@mvorisek
Copy link
Contributor Author

Your main concern was that you accidently had /dist updated files in another PR some weeks ago, where i asked you to remove them.

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 dist/ gitignore line before the files are pushed to npm, so npm nightly/stable releases continue to work as before.

With this reasoning I belive this PR can be merged safely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/on-hold Issues and pull requests which are on hold for any reason state/wont-fix Anything which isn't going to be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants