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

Interpret paths relative to config file instead of cwd #237

Merged
merged 9 commits into from
Jul 1, 2022
Merged

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Jul 1, 2022

Previously, the server root and the benchmark paths were interpreted relative to the current working directory. This meant that a given config could only ever run from one specific working directory, which was confusing and inconvenient. Now, they are interpreted relative to the config file path itself, so the current directory doesn't matter.

Also:

  • Update package locks
  • Remove outdated chromedriver pinning in CI
  • Upgraded CI to Node 16 and the latest versions of the checkout/node actions

Fixes #176

@aomarks aomarks changed the title Interpret benchmark paths relative to config file Interpret paths relative to config file instead of cwd Jul 1, 2022
Copy link
Collaborator

@AndrewJakubowicz AndrewJakubowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE!

resolveBareModules: undefined,
benchmarks: [
{
name: '/mylib/mybench/index.html',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test!

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@AndrewJakubowicz AndrewJakubowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! The only other place to maybe put a small hint of the usage of root is in the CLI docs: https://github.com/Polymer/tachometer#config-file:~:text=Show%20documentation-,%2D%2Droot,-./

@aomarks
Copy link
Member Author

aomarks commented Jul 1, 2022

Great! The only other place to maybe put a small hint of the usage of root is in the CLI docs: https://github.com/Polymer/tachometer#config-file:~:text=Show%20documentation-,%2D%2Droot,-./

The new changes only apply to the JSON configs, and when you're using a JSON config the --root flag errors (because all the config is expected to be in the JSON file).

@aomarks
Copy link
Member Author

aomarks commented Jul 1, 2022

Great! The only other place to maybe put a small hint of the usage of root is in the CLI docs: https://github.com/Polymer/tachometer#config-file:~:text=Show%20documentation-,%2D%2Droot,-./

The new changes only apply to the JSON configs, and when you're using a JSON config the --root flag errors (because all the config is expected to be in the JSON file).

Side note, in general the config file options aren't really properly documented, the README needs a revamp to make this all more clear. But not tackling that right now.

@aomarks aomarks merged commit dfd623f into main Jul 1, 2022
@aomarks aomarks deleted the config-path branch July 1, 2022 20:30
aomarks added a commit to lit/lit that referenced this pull request Jul 1, 2022
This PR removes Lerna entirely. Now we just use npm workspaces + wireit.

### Benefits

- No more `npm run bootstrap`, just `npm ci`.
- To upgrade or add a dependency, edit the `package.json` and `npm i` from the root.
- Cuts around 30s off install time in CI.
- Only one `package-lock.json` file for the whole monorepo.

### Tricky parts

- npm workspaces hoists packages to the top-level `node_modules/` folder whenever possible. This is different to Lerna, which always put all the dependencies in each package's own `node_modules/` folder. Only if there is a conflicting version would there now be a `node_modules/` folder inside a workspace package.

  - This required updating various paths to reach further up the file system.
 
  - In a couple of cases, this caused issues with conflicting `@types` packages for TypeScript, because while previously only the local version was available, now multiple versions could be. This is because TypeScript by default ambiently loads all `node_modules/@types/*` packages from _all recursive parent directories_. The solution to these cases are to specify that only the immediate `node_modules/` folder should be consulted through the `typeRoots` setting.

  - This required changing the tachometer server root directory as well, which ended up being more complicated than it should be, because of the way tachometer was treating paths in config files as relative to the current working directory, instead of the config file path. In google/tachometer#237 I changed that behavior, so this also bumps the tachometer version and updates the configs accordingly.

Fixes #3093
"root": "../..",
"benchmarks": [
{
"url": "foo.html"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[no action needed]. Ooops I missed leaving this comment. Sorry! I realized the bottom example doesn't match the top one. It doesn't have to match, however should the url match in the second example - to show an easy way to migrate using the root?

This second example currently resolves to <repo>/foo.html.

Expected the second example to be:

{
  "root": "../..",
  "benchmarks": [
      {
        "url": "benchmarks/foo/index.html"
      }
    ]
  }

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

Successfully merging this pull request may close these issues.

"root" configuration is not used when resolving a benchmark's URL
3 participants