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

Questions regarding development steps #5910

Closed
2 tasks done
sisp opened this issue Aug 28, 2023 · 3 comments
Closed
2 tasks done

Questions regarding development steps #5910

sisp opened this issue Aug 28, 2023 · 3 comments
Labels
documentation Issue concerns the documentation

Comments

@sisp
Copy link
Contributor

sisp commented Aug 28, 2023

Description

I've stumbled over some issues when contributing to mkdocs-material and following the theme development instructions. When I set up the environment and then, without modification of the sources, build the theme, I see the following git status output:

$ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	deleted:    material/.overrides/assets/javascripts/custom.149a65e0.min.js
	deleted:    material/.overrides/assets/javascripts/custom.149a65e0.min.js.map
	deleted:    material/.overrides/assets/javascripts/iconsearch_index.json
	deleted:    material/.overrides/assets/stylesheets/custom.f7ec4df2.min.css
	deleted:    material/.overrides/assets/stylesheets/custom.f7ec4df2.min.css.map
	deleted:    material/.overrides/home.html
	deleted:    material/.overrides/hooks/translations.html
	deleted:    material/.overrides/hooks/translations.py
	deleted:    material/.overrides/main.html

Since I made no changes, I would have expected an empty change list.

When I run npm run build:all instead, I get this:

$ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	deleted:    material/.overrides/assets/javascripts/iconsearch_index.json

Why has his file been deleted? 🤔

And when I make a small change in, e.g., src/assets/stylesheets/main/integrations/_mermaid.scss and src/assets/javascripts/components/content/mermaid/index.css (while working on #5824) and run npm run build:all, I get this:

$ git add .
$ git status
On branch feat/mermaid-sequence-diagram-styles
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	deleted:    material/.overrides/assets/javascripts/iconsearch_index.json
	renamed:    material/assets/javascripts/bundle.78eede0e.min.js -> material/assets/javascripts/bundle.db3e830e.min.js
	renamed:    material/assets/javascripts/bundle.78eede0e.min.js.map -> material/assets/javascripts/bundle.db3e830e.min.js.map
	renamed:    material/assets/stylesheets/main.0e669242.min.css -> material/assets/stylesheets/main.f762398d.min.css
	renamed:    material/assets/stylesheets/main.0e669242.min.css.map -> material/assets/stylesheets/main.f762398d.min.css.map
	modified:   material/base.html

Why have the built/minified CSS (map) files been renamed? I would have expected their file contents to have changed because their sources have changed.

And finally, why have the built/minified JS (map) files been renamed? I haven't touched their sources at all.

Could you please help me understand what's going on, @squidfunk? 🙏

Related links

Proposed change

No response

Before submitting

@sisp sisp added the documentation Issue concerns the documentation label Aug 28, 2023
@sisp
Copy link
Contributor Author

sisp commented Aug 28, 2023

Strange, regarding the renamed CSS/JS (map) files, I just noticed there are indeed some changes, but Git marks them only as "renamed" – odd. The CSS file actually contains the changes I made in the corresponding sources. 👍 And the JS minification seems to be non-deterministic – minified identifiers aren't consistent across builds. So it seems the "renamed" built assets are fine minus being declared as "renamed" although they're actually "modified" and the slightly unnecessary diff noise due to the JS minification.

@squidfunk
Copy link
Owner

Thanks for asking! As a small primer:

  • npm run build builds the theme without our overrides. This is normally what you should use when you fork the theme and apply your own changes, because you normally don't want our overrides
  • npm run build:all builds the theme with our overrides.

Thus, when you create a PR, always use npm run build:all.

Why has his file been deleted? 🤔

deleted:    material/.overrides/assets/javascripts/iconsearch_index.json

This may be surprising as it's not documented, because it is specific to our installation. Take the following pipe:

/* Compute emoji mappings (based on Twemoji) */
const emojis$ = defer(() => resolve("venv/**/twemoji_db.py"))
.pipe(
switchMap(file => read(file)),
map(data => {
const [, payload] = data.match(/^emoji = ({.*})$.alias/ms)!
return Object.entries<TwemojiIcon>(JSON.parse(payload))
.reduce((index, [name, { unicode }]) => index.set(
name.replace(/(^:|:$)/g, ""),
`${unicode}.svg`
), new Map<string, string>())
})
)

This observable will only emit when the twemoji_db.py file is found in a folder called venv. This means that you need to create a virtual environment in venv and install all dependencies of the theme. The index will then be generated. It's a little hacky, because it has several assumptions (folder name, user installed dependencies, etc.), but it's like that for simplicity – normally, it's not necessary for users to generate it, so throwing an error will lead to more issues and questions than just ignoring it. Economics of maintaining an Open Source project 101.

How could we improve it? First, we could align the name of the folder with the docs where we mention .venv. Historically, I've been using venv(without the dot), and if you don't want to give me a hard time, we should maybe change what the docs recommend to venv 😅 It's only a preference, after all, because you'd need to add the folder to .gitignore anyway. Next, we should mention that the author should actually install the requirements, and we've already been doing that for ages, but some people just don't read carefully enough, making more work for us answering questions. So, we could print a warning in the tools that the icon search index was not generated and instruct the user to install the requirements before creating a PR. I'm not sure I want to tie our tool chain too much to PR creation, as several users are using the same chain downstream.

So, not sure.

Why have the built/minified CSS (map) files been renamed? I would have expected their file contents to have changed because their sources have changed.

Well, you changed the sources, and the files with those funny looking hashes at the end are actually generated files (aka entrypoints). Since the files you changed are included by those files, their contents also changed, thus leading to new hashes. Why do we use hashes? That you don't have to tell your users to "trigger a full reload" to get the latest changes 😅 Ah well, happy old times. It's actually common practice, and all tools that bundle for the web recommend doing that.

And finally, why have the built/minified JS (map) files been renamed? I haven't touched their sources at all.

That's a question you should ask a git professional. I think because the changes are so tiny compared to the size of the file, git considers the action to be a rename rather than a delete and add. But that's only my best guess.

And the JS minification seems to be non-deterministic – minified identifiers aren't consistent across builds.

That's something I'm hearing for the first time. It might be related to ordering of files, but that should be deterministic as well. We're using ESBuild and SASS for bundling, so maybe check back with the authors and maintainers of those projects if you have further questions regarding determinism. We're only taking the final output by those tools and compute a hash based on the files contents.

I hope I could answer all questions and am closing this issue as resolved. If you have further questions, please use the discussion board ☺️

@sisp
Copy link
Contributor Author

sisp commented Aug 29, 2023

Thanks for your detailed explanations! 🙇

I've submitted #5919 to update the docs about the name of the virtual env directory and some additional minor tweaks. I've also submitted #5920 to update the icon search index as it seems to have been outdated – a clean installation of the dependencies and running npm run build:all resulted in an updated index file.

What do you think about also locking Python dependencies for development to make development environments deterministic? You're already using package-lock.json for Node.js dependencies since NPM supports dependency locking out of the box. But Python's pip doesn't; various alternatives have emerged, Poetry being the most popular to my knowledge and judging by GitHub stars and contributors. Dependabot also supports maintaining the lock file. For what it's worth, I've been using Poetry for several years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issue concerns the documentation
Projects
None yet
Development

No branches or pull requests

2 participants