package-lock.json file is already put into .gitignore, but it is still inside the repository. For starterkit we should let the people decide whether they want to use npm, yarn or somerhing else. Also it is created with some older version of node and it gets overriden after running npm install locally, which may be confusing.
I would vote also to remove the compiled dist directory out of repo and follow that with a solid statement in the README file saying that you have to run npm build at least once to let the theme work properly. This is a starterkit boilerplate, not a ready theme, so such an approach sounds more adequate. Keeping a compiled dist dir in a repo will also add an unnecessary noise to the git repository and may cause potential merge conflicts.
Putting dist dir into .gitignore makes sense because it encourages a more CI style of work, and also make this project easier to maintain. However, we should put inside a .gitignore a commentary in a following style:
# Ignore compiled assets - remove it if you prefer to commit compiled css into the git
/dist/Issue fork tailwindcss-3578746
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
ravi kant commentedThanks for the detailed feedback.
Good point about the `package-lock.json`. Since this is a starterkit, it makes sense not to enforce a specific package manager. Removing it from the repository would allow users to choose whether they want to use npm, yarn, or another tool without confusion caused by an outdated lock file.
Regarding the `dist` directory, we could keep the initial compiled assets in the repository to avoid the theme appearing broken immediately after enabling it. However, we could add the `dist` directory to `.gitignore` to prevent merge conflicts during development.
Happy to hear other opinions on this approach.
Comment #3
firflantAgree that we should definitely avoid anyone having a feeling of a broken theme. Adding dist file to gitignore, but not removing it from a project repo seems like a win-win approach.
Comment #5
firflantIntroduced the change as-discussed.
Comment #6
firflantOne more point just came to my mind. If we keep the dist dir commited inside the repo, maybe we should disable the --minify flag on a build command? Keeping the minification enabled decreases the readability of a git repository for this project and for any site that will decide to go with the "commit the generated stylesheet in" variant. Changing just one line in a source css regenerates the entire bundle. With minification enabled we get much smaller diffs.
At the end, Drupal does css minification by itself so we can safety turn it off on a tailwind level.
Comment #7
ravi kant commentedAccording to me, if we continue working this way, we will encounter conflicts as usual. However, our focus is only to ensure that the page does not appear broken when this theme is set as the default. If we remove the --minify flag, users will have to enable optimization themselves, which would be considered a limitation of the theme.
Comment #8
firflantRight, also tailwind and postcss ecosystem provides a better minification than the Drupal core.
Minification flag was not turned down by this MR - so I am moving it for review.
Comment #10
ravi kant commentedComment #11
firflantComment #13
firflantComment #14
ravi kant commentedComment #16
firflant@ravi-kant Please update the credit to the contributors in the contribution record.