-
Notifications
You must be signed in to change notification settings - Fork 247
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
Bundle the font #412
Bundle the font #412
Conversation
For performance, offline support and privacy. Cut down the CSS file from the package so that we only need to bundle the one font file, like what Google was doing.
d30c418
to
b7d026e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and confirmed that Parcel indeed knows to copy and bundle the file from node_modules
.
Opening index.htm from the latest stable release zip, involves two network requests to fonts.googleapis.com
. Checking out this patch, running scripts/prepare-test-installation.sh
, and opening dist/release/index.html involves no network requests.
Thank you! Next time I'm ready to cut a release, I'll test this myself and merge |
Hi @jlfwong, thanks for your response. @Krinkle and I are on the performance team at Wikimedia and we're looking at integrating speedscope with Wikimedia websites using our own sampling profiler for PHP. We're doing it in a way that should be usable by other PHP applications. The details are at https://phabricator.wikimedia.org/T291015 |
When I look at https://fonts.googleapis.com/css?family=Source+Code+Pro, it lists the I'm also not sure what the licensing requirements of this become once it's bundled with the software. From looking at https://github.com/adobe-fonts/source-code-pro/blob/release/LICENSE.md, it seems like I'm supposed to include licensing information somewhere in this project. If you're interested in having this merged, can you please look into that and see what, if anything, would need to change in this repository? |
I didn't realise that it delivers different stylesheets depending on User-Agent. If you use curl, it only shows TTF.
Yes, I think you're right that a license notice is required. Although probably all of your runtime dependencies have this problem. |
Gotcha. As far as I can tell, WOFF-2 is very widely supported now (https://caniuse.com/?search=woff2), so I'd prefer this woff-2 instead to keep things as compact as possible.
This might be true, but AFAIK all of my other runtime dependencies have common software licenses like MIT, rather than a font-specific license. I've never heard of folks getting in trouble for bundling MIT licenses for npm dependencies (otherwise every single npm package would need this). But I know much less about legal issues around fonts. |
Stuff about MIT and FLOSS licensesI am not a lawyer, but my understanding based on working in open source at Wikimedia Foundation and in the jQuery Team (OpenJS/Linux Foundation) for over a decade, is as follows: The MIT license is among the most liberal in open source. Practically the closest thing to being entirely in the public domain. You can do literally anything you want with it, as long as you attribute its authors/copyright line somewhere. Similar in spirit to the Creative Commons Attribution license in that sense. "Attribution", thus requires the copyright line to be preserved or otherwise presented in distributions that include copies of the software. Most npm packages are not end-user applications or products, and thus don't have any practical involvement around this. For a typical library package, its only concern is its own license in the repo/package uploaded to npm, and anyting it uses or that others use it for, is handled transparently by npm-install later on when someone else uses it. When you upload your own package to npm, usually, you are not distributing your dependencies for they are not included in the package. If you do something special with the code and distribute it by other means outside npm, or if you attach artefacts to your npm package that bundle/embed/inline code from dependencies, then you have to preserve or present the attribution by other means yourself. For example:
In the case of Speedscope, Parcel is used to produce a single JS file and a single CSS file, and any comments are stripped. My best guess is that this is technically in violation of the MIT license for your dependencies. When I publish a copy of Speedscope on my own domain, I'm (probably?) not in violation of your license, since the Speedscope UI links to your GitHub repo, where your license file can be viewed. However, there's not really a clear path to the copyright statement of your dependencies (much less which ones were bundled, or which versions, or which copyright statement they had at that time). Parcel strips it all out. I could not find anything in the the Speedscope release tarball, e.g. the HTML, JS file, or other text files, that indicates what code was bundled or who holds their copyright. The source repo also does not distinguish in package.json between dev and prod dependencies (noting that your dev dependencies are generally not distributed. E.g. the tools you use for bundling, testing, minification are not being distributed in the release and do not require attribution). Instead, it seems your prod dependencies are listed among dev-only, and some of them are then bunded into the JS file during release. This makes sense for efficiency, but means that the npm distribution is probably technically in violation as well. FontAs for the font, https://openfontlicense.org/ofl-faq/ does a good job of answering the hard questions. It is in essense an even more liberal license than the MIT license, as it presents plethora of scenarios where you do not even have to attribute its author or copyright holders. Unlike commerically licensed fonts where you may have to pay to download them, and then pay pay for permission to use the font in a brand or publication. The OFL is much like the MIT license in that it permits copying, modification and re-distribution, so long as you preserve a copy of the original license text and/or other attribution. In addition, it grants very useful and practical exemptions for work that merely applies the font (e.g. a logo or a screenshot of Speedscope where the font is used), where it requires no attribution at all. Given that Speedscope releases would include a copy of the file as-is, atttribution would be required, however. While by no means required, I would personally choose the following as the simplest approach:
If this sounds acceptable, I'd be happy to submit a PR doing that. |
Hey @Krinkle! Thanks for the thorough rundown, and especially for hunting down the licensing details of source-code-pro! Regarding the technical MIT violations, I think you're probably right too. That sounds, frankly, like a pain to resolve, and certainly a separate concern from what this PR is addressing. So for now I think I'll punt on that. The simplest approach you propose sounds good to me, and I'd be happy to accept a PR doing that. |
For performance, offline support and privacy. Cut down the CSS file from the package so that we only need to bundle the one font file, similar to what Google Fonts did. Ref https://openfontlicense.org/ofl-faq/ Ref https://software.sil.org/fonts/faq/ Closes jlfwong#412
For performance, offline support and privacy. Cut down the CSS file from the package so that we only need to bundle the one font file, similar to what Google Fonts did. Ref https://openfontlicense.org/ofl-faq/ Ref https://software.sil.org/fonts/faq/ Closes jlfwong#412 Co-authored-by: Timo Tijhof <[email protected]>
For performance, offline support and privacy. Continues from #412
Closing in favor of #472 |
commit d69f3d0 Author: Tom Levy <[email protected]> Date: Tue Apr 16 06:50:13 2024 +1200 Fix bug where import after error continues failing (jlfwong#463) Steps to reproduce: 1. Open https://www.speedscope.app/ 2. Try to import an invalid file such as [invalid.json](https://github.com/jlfwong/speedscope/files/14167326/invalid.json). The page says "Something went wrong. Check the JS console for more details." 3. Now try to import a valid file such as [simple.json](https://github.com/jlfwong/speedscope/files/14167335/simple.json). The page says "Something went wrong. Check the JS console for more details." even though this second file is valid. Explanation of the fix (copied from the commit message): > We need to clear the error flag, otherwise once there is an error and we display "Something went wrong" we will keep displaying that forever even when the user imports a valid file. commit 25f671e Author: Timo Tijhof <[email protected]> Date: Mon Apr 15 19:45:12 2024 +0100 Bundle the font (jlfwong#472) For performance, offline support and privacy. Continues from jlfwong#412 commit 0121cf9 Author: Tom Levy <[email protected]> Date: Tue Feb 6 07:58:18 2024 +1300 Clarify specification of startValue in speedscope file format (jlfwong#464) The previous description ("event values will be relative to this startValue") was ambiguous. Suppose the profile starts at wall time 1000ms and the first event is at wall time 1003ms. The intention is that startValue should be set to 1000 and the "at" value of the event should be set to 1003. The viewer's time axis will start at 0ms and the first event will be displayed at 3ms. But the previous description could be incorrectly interpreted as saying that the "at" value of the first event should be set to 3 (the time relative to startValue, as opposed to the absolute wall time). Clarify that "relative" is referring to how the viewer displays the data, not about which values to store in the file. commit 68fd88c Author: Jamie Wong <[email protected]> Date: Fri Jan 12 09:57:30 2024 -0800 1.20.0 commit a6d7001 Author: Zachary Marion <[email protected]> Date: Wed Jan 3 13:04:22 2024 -0800 Partition based on samples instead of traceEvents when importing a sample-based chrome trace (jlfwong#460) In the existing code, if `traceEvents` did not have any importable events, the profile would be marked as empty. This was a bug, as the dev Hermes profiles I was testing with had one X event which made the code work. However we do not need to guarantee (and the spec doesn't seem to) any traceEvents being present as long as we have samples and stack frames. When I tested a production profile taken from Hermes it did not have any importable events, just a metadata event with a thread name. This PR updates the implementation to iterate over partitioned samples instead of traceEvents so we construct profiles for all thread + process pairs referenced in the samples array. | Before | After | | --- | ----- | | <img width="1454" alt="Screenshot 2024-01-03 at 9 58 56 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/78c098a7-b374-4492-ad13-9ca79afdb40c"> | <img width="1450" alt="Screenshot 2024-01-03 at 9 58 17 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/d2d5e82b-fa3e-43db-bf8a-e8c3b84cd13a"> | commit a3c0b15 Author: Jamie Wong <[email protected]> Date: Wed Dec 27 23:57:14 2023 -0500 1.19.0 commit e9a17d5 Author: Zachary Marion <[email protected]> Date: Wed Dec 27 23:56:27 2023 -0500 Improve hermes profile frame keys to better group frames (jlfwong#459) Because all args are serialized in the key for hermes profiles, frames were not properly getting grouped since the "parent" property was different. This PR ensures that the frame key is properly constructed from the function name, file name, line + column number. | Before | After | | ----- | ----- | | <img width="1727" alt="Screenshot 2023-12-27 at 2 25 32 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/4ec04653-5a80-4f34-9506-48e0eb415983"> | <img width="1728" alt="Screenshot 2023-12-27 at 2 26 02 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/c3edc447-e8ad-4757-8576-cbb8c27eafe3"> | | <img width="1720" alt="Screenshot 2023-12-27 at 2 13 37 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/10194aba-8f26-48ca-bf15-06917b44ea99"> | <img width="1728" alt="Screenshot 2023-12-27 at 2 14 20 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/75c32401-1705-4b23-b71a-5f0a720160dd"> | commit 3f3da22 Author: Jamie Wong <[email protected]> Date: Wed Dec 27 10:48:07 2023 -0500 Update README.md - Add link to Hermes import instructions - Re-order import sources higher in the list - Update first line commit bea0ef6 Author: Jamie Wong <[email protected]> Date: Tue Dec 26 17:12:59 2023 -0500 1.18.0 commit 4feb1e5 Author: Zachary Marion <[email protected]> Date: Tue Dec 26 17:00:18 2023 -0500 Add hermes-specific support for the trace event format (jlfwong#458) Profiles that are transformed into the hermes trace event format are guaranteed to have specific arguments that supply metadata that is useful for debugging - namely the filename, line + col number that the function call originated from, with sourcemaps applied. This PR adds specific support for this information to the trace event importer. This means that we can have the Frame name be just the name of the function, since we know all the information we want to be displayed in the UI is captured in the frame info, which makes the traces cleaner to look at. | Before | After | |-----|-----| | <img width="1728" alt="Screenshot 2023-12-26 at 2 40 01 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/f9dff608-5df3-4098-b1f8-91a69185d906"> | <img width="1726" alt="Screenshot 2023-12-26 at 2 39 13 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/b8ff360e-a316-4bef-8ebc-620c9ff1a998"> | | <img width="1728" alt="Screenshot 2023-12-26 at 2 41 03 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/127a49b5-458e-4ac8-934a-202e565cb20f"> | <img width="1728" alt="Screenshot 2023-12-26 at 2 41 29 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/ebb285ce-6b33-4535-8e45-b9ada4e4d97f"> | commit 88f4fe6 Author: Jamie Wong <[email protected]> Date: Mon Dec 25 22:52:46 2023 -0500 Update README-ADMINS.md with npm login instructions (jlfwong#457) commit 60f1812 Author: Jamie Wong <[email protected]> Date: Mon Dec 25 22:40:40 2023 -0500 1.17.0 commit 1717fec Author: Jamie Wong <[email protected]> Date: Mon Dec 25 22:37:45 2023 -0500 Upgrade prettier, update prettier & react-hooks eslint plugins (jlfwong#456) Re-ran prettier with latest version commit c296f53 Author: Jamie Wong <[email protected]> Date: Mon Dec 25 22:23:33 2023 -0500 Upgrade typescript & eslint to latest, fix resulting errors (jlfwong#455) Also updated the ci.yml node test versions to 18.x and 20.x, given current support: https://endoflife.date/nodejs commit 8e0fa58 Author: Jamie Wong <[email protected]> Date: Mon Dec 25 21:22:56 2023 -0500 Re-enable eslint prettier rule after being accidentally disabled for 3 years (jlfwong#454) It looks like in jlfwong#267 (which was 3 years ago!), I accidentally disabled prettier linting altogether 😱 https://github.com/jlfwong/speedscope/pull/267/files#diff-e2954b558f2aa82baff0e30964490d12942e0e251c1aa56c3294de6ec67b7cf5 There's no comment in that PR about this being an intentional thing, so I have to assume this was a dumb mistake. commit b214804 Author: Zachary Marion <[email protected]> Date: Mon Dec 25 21:11:45 2023 -0500 Support the chrome JSON trace format (allows viewing of hermes traces) (jlfwong#453) commit dfd3a0d Author: Zachary Marion <[email protected]> Date: Sat Dec 16 00:27:03 2023 -0800 Fix bug in selectQueueToTakeFromNext for trace profiles (jlfwong#450) I have been taking a lot of profiles using the Hermes profiler, but I noticed that they sometimes to not show up properly. After debugging what exactly was going on, I realized it was because the logic in `selectQueueToTakeFromNext` only checks for name, instead of the key for the event. I had a bunch of events with the name `anonymous` that were getting improperly exited before they should have been due to this logic. This fix makes the code more robust if there are added "args" which differentiate an event from another (as is the case in Hermes profiles), however it would still be an issue if they key just defaults to the name. Example profile before: <img width="1728" alt="Screenshot 2023-12-15 at 12 54 04 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/345f556e-f944-40f1-b59c-748133acb950"> What it should look like (in Perfetto): <img width="1051" alt="Screenshot 2023-12-15 at 8 51 38 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/7473cdf8-95f1-49de-a0c7-ef4ac081ff85"> After the fix: <img width="1728" alt="Screenshot 2023-12-15 at 12 54 29 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/56b0870a-538b-4916-acc8-de2b7dfd78eb"> commit ac4a015 Author: Jamie Wong <[email protected]> Date: Thu Dec 7 18:43:47 2023 -0800 Add bounds checking for sampleTypeIndex (jlfwong#449) Wow this was surprising. As reported in jlfwong#448, the `simple.speedscope.json` file failed import. This was surprising to me because there's a test that covers that file to ensure it imports correctly. The file provided in jlfwong#448, however, is from a version of speedscope from 5 years ago before the file had been pretty printed. It turns out that when this *particular* file is not pretty-printed, it's a schematically valid pprof profile. The fix is to do some bounds checks and return null. After the change, the file imports as you'd expect after realizing its not actually a valid pprof profile. Fixes jlfwong#448
For performance, offline support and privacy.
Cut down the CSS file from the package so that we only need to bundle
the one font file, like what Google was doing.