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

Separate core search logic with search ui #126183

Merged
merged 1 commit into from
Aug 31, 2024
Merged

Conversation

Folyd
Copy link
Contributor

@Folyd Folyd commented Jun 9, 2024

Currenty, the search.js mixed with UI/DOM manipulation codes and search logic codes, I propose to extract the search logic to a class for following benefits:

  • Clean code. Separation of DOM manipulation and search logic can lead better code maintainability and easy code testings.
  • Easy share the search logic for third party to utilize the search function, such as Rust Search Extension, https://query.rs.

This PR added a new class called DocSearch, which mainly expose following methods:

class DocSearch {
	// Dependency inject searchIndex, rootPath and searchState
	constructor(rawSearchIndex, rootPath, searchState) {
		// build search index...
	}

	static parseQuery(userQuery) {
	}

	async execQuery(parsedQuery, filterCrates, currentCrate) {
	}
}

@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2024

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 9, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@notriddle
Copy link
Contributor

Separating the logic into a class makes sense, as long as it doesn't impose another HTTP request on the actual browser.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 10, 2024

☔ The latest upstream changes (presumably #126205) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Folyd
Copy link
Contributor Author

Folyd commented Jun 10, 2024

Hi, @notriddle do you know how to fix this?

/checkout/src/librustdoc/html/static/js/search-core.js
  3321:5  error  'exports' is not defined  no-undef

@rust-log-analyzer

This comment has been minimized.

@Folyd Folyd marked this pull request as ready for review June 10, 2024 22:21
@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2024

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 10, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@Folyd Folyd changed the title WIP: Move independent search logic to search-core.js Move independent search logic to search-core.js Jun 10, 2024
@rust-log-analyzer

This comment has been minimized.

@Folyd
Copy link
Contributor Author

Folyd commented Jun 11, 2024

I'm not sure how to fix this error:

Testing stage2 rustdoc-js-std (x86_64-unknown-linux-gnu)
  TypeError: exports.DocSearch is not a constructor
      at initSearch (/checkout/obj/build/x86_64-unknown-linux-gnu/doc/static.files/search-5cb5038de88a08b8.js:5:8904)
      at /checkout/obj/build/x86_64-unknown-linux-gnu/doc/static.files/search-5cb5038de88a08b8.js:5:9076
      at Object.<anonymous> (/checkout/obj/build/x86_64-unknown-linux-gnu/doc/static.files/search-5cb5038de88a08b8.js:5:9100)
      at Module._compile (node:internal/modules/cjs/loader:1218:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1272:10)
      at Module.load (node:internal/modules/cjs/loader:1081:32)
      at Module._load (node:internal/modules/cjs/loader:922:12)
      at Module.require (node:internal/modules/cjs/loader:1105:19)
      at require (node:internal/modules/cjs/helpers:103:18)
      at loadSearchJS (/checkout/src/tools/rustdoc-js/tester.js:436:26)

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

The GUI tests are not supposed to fail since you're only moving code to end up with the same output. So what I recommend is opening the test and look what it's checking so you can see what is going wrong with your changes.

@notriddle
Copy link
Contributor

notriddle commented Aug 27, 2024

In particular, try:

  • running the GUI tests:
    ./x.py test tests/rustdoc-ui (edit: gui, not ui)
    • it'll prompt you to npm install its dependencies, if you don't have them
  • if they fail, try opening the test case and reproducing it in your browser:
    xdg-open build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/lib2/index.html
    • on Windows, xdg-open is just start, and on macos it's just open
  • if you get browser console errors, replace the script in rustdoc-gui with a non-escaped one:
    cp src/librustdoc/html/static/js/search.js build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/static.files/search-*

@Folyd
Copy link
Contributor Author

Folyd commented Aug 28, 2024

Thanks @notriddle. The GUI test is success on my macOS.

$ ./x.py test tests/rustdoc-ui
Building bootstrap
    Finished `dev` profile [unoptimized] target(s) in 0.43s

Creating a sysroot for stage2 compiler (use `rustup toolchain link 'name' build/host/stage2`)
Building stage0 library artifacts (x86_64-apple-darwin)
    Finished `release` profile [optimized] target(s) in 0.45s
Building stage0 tool compiletest (x86_64-apple-darwin)

Testing stage2 compiletest suite=rustdoc-ui mode=ui (x86_64-apple-darwin)

running 282 tests
iii................................................i....................................  88/282
........................................................................................ 176/282
......i.............i......i............................................................ 264/282
..................

test result: ok. 275 passed; 0 failed; 7 ignored; 0 measured; 0 filtered out; finished in 52.24s

Build completed successfully in 0:02:57

@notriddle
Copy link
Contributor

Oh, sorry.

You need to run ./x.py test tests/rustdoc-gui

@Folyd
Copy link
Contributor Author

Folyd commented Aug 28, 2024

Finally, all testings passed. 🥳

@GuillaumeGomez
Copy link
Member

Time to squash the commits. :D

Then I'll review more in details.

@Folyd
Copy link
Contributor Author

Folyd commented Aug 28, 2024

Do I need git squash manually?

@GuillaumeGomez
Copy link
Member

Yes. If you don't know how, I wrote a tutorial a while ago here.

@Folyd
Copy link
Contributor Author

Folyd commented Aug 28, 2024

Rebase done. Thanks for the tutorial. ❤️ @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

Looks good to me! Since all tests are passing, I'm pretty confident everything is ok. Just letting @notriddle take a look to ensure nothing was missed.

@notriddle
Copy link
Contributor

@bors r=GuillaumeGomez,notriddle

@bors
Copy link
Contributor

bors commented Aug 29, 2024

📌 Commit 1eb4cb4 has been approved by GuillaumeGomez,notriddle

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 29, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 30, 2024
…z,notriddle

Separate core search logic with search ui

Currenty, the `search.js` mixed with UI/DOM manipulation codes and search logic codes, I propose to extract the search logic to a class for following benefits:

- Clean code. Separation of DOM manipulation and search logic can lead better code maintainability and easy code testings.
- Easy share the search logic for third party to utilize the search function, such as [Rust Search Extension](https://rust.extension.sh), https://query.rs.

This PR added a new class called `DocSearch`, which mainly expose following methods:

```js
class DocSearch {
	// Dependency inject searchIndex, rootPath and searchState
	constructor(rawSearchIndex, rootPath, searchState) {
		// build search index...
	}

	static parseQuery(userQuery) {
	}

	async execQuery(parsedQuery, filterCrates, currentCrate) {
	}
}
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2024
…kingjubilee

Rollup of 8 pull requests

Successful merges:

 - rust-lang#126183 (Separate core search logic with search ui)
 - rust-lang#129366 (linker: Synchronize native library search in rustc and linker)
 - rust-lang#129403 (Ban non-array SIMD)
 - rust-lang#129527 (Don't use `TyKind` in a lint)
 - rust-lang#129534 (Deny `wasm_c_abi` lint to nudge the last 25%)
 - rust-lang#129640 (Re-enable android tests/benches in alloc/core)
 - rust-lang#129731 (Allow running `./x.py test compiler`)
 - rust-lang#129754 (wasi: Fix sleeping for `Duration::MAX`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2024
…kingjubilee

Rollup of 8 pull requests

Successful merges:

 - rust-lang#126183 (Separate core search logic with search ui)
 - rust-lang#129366 (linker: Synchronize native library search in rustc and linker)
 - rust-lang#129403 (Ban non-array SIMD)
 - rust-lang#129527 (Don't use `TyKind` in a lint)
 - rust-lang#129534 (Deny `wasm_c_abi` lint to nudge the last 25%)
 - rust-lang#129640 (Re-enable android tests/benches in alloc/core)
 - rust-lang#129731 (Allow running `./x.py test compiler`)
 - rust-lang#129754 (wasi: Fix sleeping for `Duration::MAX`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2024
…kingjubilee

Rollup of 8 pull requests

Successful merges:

 - rust-lang#126183 (Separate core search logic with search ui)
 - rust-lang#129366 (linker: Synchronize native library search in rustc and linker)
 - rust-lang#129403 (Ban non-array SIMD)
 - rust-lang#129527 (Don't use `TyKind` in a lint)
 - rust-lang#129534 (Deny `wasm_c_abi` lint to nudge the last 25%)
 - rust-lang#129640 (Re-enable android tests/benches in alloc/core)
 - rust-lang#129731 (Allow running `./x.py test compiler`)
 - rust-lang#129754 (wasi: Fix sleeping for `Duration::MAX`)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 31, 2024
…z,notriddle

Separate core search logic with search ui

Currenty, the `search.js` mixed with UI/DOM manipulation codes and search logic codes, I propose to extract the search logic to a class for following benefits:

- Clean code. Separation of DOM manipulation and search logic can lead better code maintainability and easy code testings.
- Easy share the search logic for third party to utilize the search function, such as [Rust Search Extension](https://rust.extension.sh), https://query.rs.

This PR added a new class called `DocSearch`, which mainly expose following methods:

```js
class DocSearch {
	// Dependency inject searchIndex, rootPath and searchState
	constructor(rawSearchIndex, rootPath, searchState) {
		// build search index...
	}

	static parseQuery(userQuery) {
	}

	async execQuery(parsedQuery, filterCrates, currentCrate) {
	}
}
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2024
…iaskrgr

Rollup of 15 pull requests

Successful merges:

 - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals)
 - rust-lang#126183 (Separate core search logic with search ui)
 - rust-lang#129123 (rustdoc-json: Add test for `Self` type)
 - rust-lang#129366 (linker: Synchronize native library search in rustc and linker)
 - rust-lang#129527 (Don't use `TyKind` in a lint)
 - rust-lang#129534 (Deny `wasm_c_abi` lint to nudge the last 25%)
 - rust-lang#129640 (Re-enable android tests/benches in alloc/core)
 - rust-lang#129642 (Bump backtrace to 0.3.74~ish)
 - rust-lang#129675 (allow BufReader::peek to be called on unsized types)
 - rust-lang#129723 (Simplify some extern providers)
 - rust-lang#129724 (Remove `Option<!>` return types.)
 - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries)
 - rust-lang#129731 (Allow running `./x.py test compiler`)
 - rust-lang#129751 (interpret/visitor: make memory order iteration slightly more efficient)
 - rust-lang#129754 (wasi: Fix sleeping for `Duration::MAX`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1f0292b into rust-lang:master Aug 31, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 31, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2024
Rollup merge of rust-lang#126183 - Folyd:search-core, r=GuillaumeGomez,notriddle

Separate core search logic with search ui

Currenty, the `search.js` mixed with UI/DOM manipulation codes and search logic codes, I propose to extract the search logic to a class for following benefits:

- Clean code. Separation of DOM manipulation and search logic can lead better code maintainability and easy code testings.
- Easy share the search logic for third party to utilize the search function, such as [Rust Search Extension](https://rust.extension.sh), https://query.rs.

This PR added a new class called `DocSearch`, which mainly expose following methods:

```js
class DocSearch {
	// Dependency inject searchIndex, rootPath and searchState
	constructor(rawSearchIndex, rootPath, searchState) {
		// build search index...
	}

	static parseQuery(userQuery) {
	}

	async execQuery(parsedQuery, filterCrates, currentCrate) {
	}
}
```
@Folyd Folyd deleted the search-core branch August 31, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants