-
Notifications
You must be signed in to change notification settings - Fork 19
Update docs and add example app #31
base: master
Are you sure you want to change the base?
Conversation
cc @JasonGore |
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.
Thanks for the PR! I have some questions I'd like to work out with you
optimize and minify the app. However, for the flamegraphs to be readable and meaningful, we need to disable the | ||
minification. While `react-scripts` don't allow us to tweak the `webpack` out of the box, it is possible via | ||
`react-app-rewired` package. With `react-app-rewired` added as a dev dependency, we can tell it to override certain | ||
configuration values of `webpack` such as turning minification off for the production build. |
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.
I worry this is straying to far from the concerns of a basic getting started example. I would rather leave it as it was or create a similarly simple example without using react-scripts
, if these additional steps are unavoidable.
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.
Actually, that sounds good! I'm more than happy to add an additional section to the guide, "Examples" say, or something. The main point of this is for users who do use the modern react-scripts
approach to guide them how to work with Flamegrill to get their flamegraphs out. How does that sound to you?
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.
OK, so I've now reshuffled things a little bit and reverted back the original quick start guide to what it was, and added a new section to the docs called Examples
, and moved the fluentui-cra-template-app example there. Have a look and lemme know what you reckon!
|
||
Modifying the path as appropriate, run flamegrill against `dist/index.html` generated with the build above: | ||
Modifying the path as appropriate, run Flamegrill against the URL where you serve your app with `serve`. By default, | ||
this would be `localhost:5000`: |
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.
I tend to think using a static file is better here. It shows that any arbitrary file locator works and that local dev server isn't actually needed (as is the case with various pipelines implementing flamegrill in CI.)
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.
So for some reason, I haven't got to the reason why yet, analysing static file will not work in this case -- the generated flamegraph comes back empty. Lemme have a closer look to figure out why. It seems to me that maybe the production assets generated with react-scripts
and whatever version of webpack
they wrap internally might require spawning a server instance to actually serve the site. But this is just a hunch, I'll dig in deeper and report back.
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.
OK, I figured it out. The reason nothing was loading when opening the index.html
directly (or pointing flamegrill at it directly) was because react-scripts
configures webpack
to use absolute paths for all the assets outputted from the production build. I've now applied one additional tweak to the webpack
config that's described in the guide and flamegrill works great by pointing it at the main website entrypoint.
@@ -0,0 +1,15 @@ | |||
import { ReportHandler } from 'web-vitals'; | |||
|
|||
const reportWebVitals = (onPerfEntry?: ReportHandler) => { |
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.
Is web-vitals
important to mention? Is this generated from react-scripts
?
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.
It's generated by the template, yep.
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.
I'm not sure about source controlling output from react-scripts
. It could cause confusion like mine here. Are there any other options like scripting the template generation in the example? Might be a good unit test to make sure it stays working with "live" react-scripts
rather than a snapshot of previous output.
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.
Oh, I probably didn't make myself clear last time. The web-vitals
code is part of the FluentUI CRA template (see here) and is not auto-generated by react-scripts
, hence, as such, I don't think there is much we can or should do here. If the template itself removes it, then it will disappear from the example app here as well.
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.
I just ran npx create-react-app my-app --template @fluentui/cra-template
and saw a lot of the same output that's here. It seems this code is autogenerated. Is there a reason we can't make npx create-react-app my-app --template @fluentui/cra-template
part of the steps rather than controlling its output? The point being that this example should continue to work with whatever npx create-react-app my-app --template @fluentui/cra-template
spits out, not a snapshot of its output at this point in time.
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.
Wait, but that's exactly what is happening here. The only thing we have to rewire is preventing minification which is required to be done regardless of the template used as long as create-react-app
is used for bootstrapping.
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.
We have a disconnect so if this reply doesn't clarify let's chat on the side. :)
Is examples/fluentui-app/config-overrides.js
the only modification necessary to the template's output? If so, why is it necessary to commit any of the template's output?
Let me know if this is wrong, but I'm seeing the steps for reproducing this example as:
npx create-react-app my-app --template @fluentui/cra-template
- Presence of
examples/fluentui-app/config-overrides.js
- Maybe some modifications to
package.json
?
I'm trying to figure out why it's necessary to add the output from step 1 into git control at all.
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.
No, your understanding seems bang on. So my reasoning for including the contents of the npx create-react-app
was so that first of all, all of the tweaks are applied immediately, and secondly, so that the potential user of flamegrill
has some examples that effectively allow them to simply invoke yarn
, yarn build
and yarn flamegrill
and explore the flamegraphs. This, in my opinion, lends itself well as a starting point of exploring the internals of flamegrill and flamegraphs in general, since it shields the user from the nitty-gritty details required just to get started. Let me know if that makes sense!
examples/fluentui-app/README.md
Outdated
or simply using `yarn build` alias as it's been preconfigured to use `react-app-rewired`. This will put the | ||
production build of the app in the `build` directory. A note of caution here. Simply running `flamegrill` on the | ||
resultant `build/index.html` will not do it. It is essential to start a dev server and the serve the production build, | ||
and then point `flamegrill` to the served url. |
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.
Why is not pointing to static output sufficient? This is a core characteristic of flamegrill I'd like to keep intact, as well as any examples using it.
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.
Excellent question! As I pointed out above, pointing at the static output generates empty flamegraph for some reason. Lemme have a closer look at it and see if I can figure out exactly why that this is the case. In the meantime, if you have any ideas why that might be the case, lemme know!
Thanks for the review, and absolutely, please keep the review comments coming! |
@@ -0,0 +1,15 @@ | |||
import { ReportHandler } from 'web-vitals'; | |||
|
|||
const reportWebVitals = (onPerfEntry?: ReportHandler) => { |
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.
I'm not sure about source controlling output from react-scripts
. It could cause confusion like mine here. Are there any other options like scripting the template generation in the example? Might be a good unit test to make sure it stays working with "live" react-scripts
rather than a snapshot of previous output.
3. Run Flamegrill | ||
|
||
Modifying the path as appropriate, run Flamegrill against the URL where you serve your app with `serve`. By default, | ||
this would be `localhost:5000`: |
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.
Is there still a need for localhost
reference here?
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.
Not at all, good catch!
optimize and minify the app. However, for the flamegraphs to be readable and meaningful, we need to disable the | ||
minification. While `react-scripts` don't allow us to tweak the `webpack` out of the box, it is possible via | ||
`react-app-rewired` package. With `react-app-rewired` added as a dev dependency, we can tell it to override certain | ||
configuration values of `webpack` such as turning minification off for the production build. |
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.
Concerning use of react-app-rewired
, I'm wondering if there are any other alternatives here, like ejecting?
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.
Correct, ejecting is also an option; however, my thinking here was to demonstrate that you don't need to go as far as ejecting your project to make it work with flamegrill (i.e., it's easier to add and then remove a dev dep such as react-app-rewired
than to revert ejection process).
@@ -85,4 +85,4 @@ In the directory where you run flamegrill, there should be an `AppTest.html` fil | |||
|
|||
![flamegraph](./results.png) | |||
|
|||
We can see here that `InefficientComponent` stands out quite a bit, consuming 47.62% of total render time. | |||
We can see here that `InefficientComponent` stands out quite a bit, consuming 47.62% of total render time.t |
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.
The changes in this file seem unintentional.
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.
Excellent catch, thanks!
This commit update docs to reflect the existence of the new CRA template for FluentUI that uses `react-scripts` for dev/build paths. It also adds an example FluentUI app that the user can examine while following along the 'Getting Started' section of the docs.
@@ -33,7 +33,7 @@ const InefficientComponent: React.FunctionComponent = (props) => { | |||
|
|||
In the same `src/App.tsx` file, use `InefficientComponent`: | |||
|
|||
```tsx | |||
```sx |
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.
This still seems like part of the leftover unintentional change.
or simply using `yarn build` alias as it's been preconfigured to use `react-app-rewired`. This will put the | ||
production build of the app in the `build` directory. | ||
|
||
Next, create `perf` directory where we'll store generated performance flamegraphs and logs, and run |
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.
Might be worth adding perf
to .gitignore
since it's a recommended directory.
examples/fluentui-app/README.md
Outdated
If you build the for production using | ||
|
||
``` | ||
node_modules/.bin/react-app-rewired build |
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.
If this is defined as a script entry, why not use yarn build
?
I tried yarn build
locally and it looks like there's an issue with the package.json
:
D:\git\flamegrill\examples\fluentui-app (examples-update -> kubkon)
λ yarn build
yarn run v1.22.4
error An unexpected error occurred: "D:\\git\\flamegrill\\examples\\fluentui-app\\package.json: Unexpected token } in JSON at position 1069".
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.
If this is defined as a script entry, why not use
yarn build
?
We could indeed. I just wanted to be more explicit about what is happening under-the-hood, but I'm happy to change this one to yarn build
instead.
I tried
yarn build
locally and it looks like there's an issue with thepackage.json
:D:\git\flamegrill\examples\fluentui-app (examples-update -> kubkon) λ yarn build yarn run v1.22.4 error An unexpected error occurred: "D:\\git\\flamegrill\\examples\\fluentui-app\\package.json: Unexpected token } in JSON at position 1069".
Ah, excellent catch, thanks!
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.
I think exposing what's being done here is preferable, and in that case could just be yarn react-app-rewired build
, which should work even without a scripts
entry. Fundamentally I think you should be leaning on yarn to find your deps for you rather than doing it yourself with a hardcoded path. :)
examples/fluentui-app/README.md
Outdated
|
||
``` | ||
mkdir perf && cd perf | ||
../../../node_modules/.bin/flamegrill -n AppTest -s file:///path/to/my/app/build/index.html |
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.
The pathing here is also pretty ugly. Is there a reason this can't be yarn flamegrill -n AppTest -s file:///path/to/my/app/build/index.html
?
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.
Oh, we absolutely could. I just wanted to point out that it is fine to run this example using a local build of flamegrill
itself. Lemme adjust then!
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.
That's fine, but just for clarity, I think the two most common use cases would be:
- Someone does
yarn install -g flamegrill
, in which case they should be able to invokeflamegrill
directly - Someone adds
flamegrill
as a devDep, which allows them to invoke it usingyarn flamegrill
I don't think in any case people should have to be diving into a specific node_modules
directory to invoke things. That said, I think having yarn flamegrill <args>
here is fine without the scripts entry, if you want to expose all of the args.
Comments addressed @JasonGore, ready for the next round of reviews! |
@@ -0,0 +1,15 @@ | |||
import { ReportHandler } from 'web-vitals'; | |||
|
|||
const reportWebVitals = (onPerfEntry?: ReportHandler) => { |
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.
We have a disconnect so if this reply doesn't clarify let's chat on the side. :)
Is examples/fluentui-app/config-overrides.js
the only modification necessary to the template's output? If so, why is it necessary to commit any of the template's output?
Let me know if this is wrong, but I'm seeing the steps for reproducing this example as:
npx create-react-app my-app --template @fluentui/cra-template
- Presence of
examples/fluentui-app/config-overrides.js
- Maybe some modifications to
package.json
?
I'm trying to figure out why it's necessary to add the output from step 1 into git control at all.
examples/fluentui-app/package.json
Outdated
"build": "react-app-rewired build", | ||
"test": "react-scripts test", | ||
"eject": "react-scripts eject", | ||
"flamegrill": "./../../node_modules/.bin/flamegrill -t perf -o perf" |
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.
The node_modules
pathing really shouldn't be needed anywhere. Install location can vary depending on package manager and hoisting status, so using node_modules
pathing at all is generally an antipattern.
I believe this should just work as "flamegrill": "flamegrill -t perf -o perf"
since flamegrill
is a dep. Although this raises an interesting question: why is flamegrill
not a devDep?
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.
Ah, excellent point, thanks!
Please consider not rebasing / force pushing every time you update this branch. It makes pulling updates painful. Commits are squashed anyways, so doing rebases / force pushes doesn't really add any benefit. |
# yarn lockfile v1 | ||
|
||
|
||
"@babel/code-frame@7.10.4": |
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.
I tried running yarn build
in examples/fluentui-app
and it failed. Then I noticed this yarn.lock
.
This repo is set up as a monorepo, so you should only have to run yarn
in repo root. This yarn.lock
file shouldn't exist, and users shouldn't have to run yarn
in subpackages. Please remove this file, add examples
to lerna.json
, do a git clean -fdx
in repo root, run yarn
and confirm everything runs in the example as you expect.
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.
Oh, sure, will do! However, git clean -dfx
is not a good suggestion to give to contributors - if someone is not very well versed with git, this will delete changes that were not staged yet effectively undoing their work. It is almost always better to hint on using interactive mode instead git clean -dix
.
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.
I'm not giving this advice to random contributors, I'm giving it to you, and you should be well versed in git.
This commit updates docs to reflect the existence of the new CRA template
for FluentUI that uses
react-scripts
for dev/build paths. It also addsan example FluentUI app that the user can examine while following along
the 'Getting Started' section of the docs.