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

Support new alias feature from neo4j 4.4 #1605

Merged
merged 11 commits into from
Nov 23, 2021

Conversation

OskarDamkjaer
Copy link
Contributor

@OskarDamkjaer OskarDamkjaer commented Nov 18, 2021

Adds support for the 4.4 concept of aliases.
To create an alias run create alias myAlias for database neo4j.

You should not be able to run :use myAlias and see your aliases when running :dbs. The "real" name should still be displaced in editor.

Preview @http://support_aliases.surge.sh

Copy link
Contributor

@jchen042 jchen042 left a comment

Choose a reason for hiding this comment

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

Good job!
I have two minor comments in the diff. As we paired together last Thursday, I can understand why you made those changes much quicker.
In addition, it would be great to include unit test and E2E test on this scenario if you've confirmed the UI behaviour with PM?

src/browser/modules/App/App.tsx Outdated Show resolved Hide resolved
src/shared/services/commandInterpreterHelper.ts Outdated Show resolved Hide resolved
@OskarDamkjaer
Copy link
Contributor Author

Thanks for the comments, made a few improvements now. Also pushed the last part of the feature, only have testing left to do. I'll re-request your review when ready 🙏

@OskarDamkjaer OskarDamkjaer changed the title Support new alias feature from neo4j 4.4 [wip] Support new alias feature from neo4j 4.4 Nov 23, 2021
@@ -505,7 +505,6 @@ export const startupConnectEpic = (action$: any, store: any) => {
)

if (
//TODO Considder the SSO implications of this
!(discovered && discovered.hasForceURL) && // If we have force url, don't try old connection data
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old comment left in by mistake

@@ -105,8 +105,6 @@ describe('getAndMergeDiscoveryData', () => {
expect(discoveryData).toBeTruthy()
expect(discoveryData?.host).toEqual(boltHost)

// expect(discoveryData?.source).toEqual(DISCOVERY_ENDPOINT)
// Todo test source via logs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

another old comment left in by mistake

@@ -17,6 +17,7 @@
"noFallthroughCasesInSwitch": true,
"jsx": "react",
"outDir": "./build",
"noEmit": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set tsc to not emit any javascript when it runs, since we don't use tsc for compiling but we get weird errors when running the tests if we have accidentally have generated some js code

@OskarDamkjaer OskarDamkjaer changed the title [wip] Support new alias feature from neo4j 4.4 Support new alias feature from neo4j 4.4 Nov 23, 2021
Copy link
Contributor

@jchen042 jchen042 left a comment

Choose a reason for hiding this comment

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

LGTM! 💯 Are we going to add E2E tests in TeamCity against 4.4.x soon?

@OskarDamkjaer
Copy link
Contributor Author

As soon as it's released!

Copy link
Contributor

@eijawerner eijawerner left a comment

Choose a reason for hiding this comment

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

Great work! Nice to see a lot of ts fixes and general cleanup of code. ⭐ See my comments that are mostly stuff that you can do if you agree and some question marks.

src/browser/modules/DBMSInfo/DatabaseSelector.tsx Outdated Show resolved Hide resolved
src/browser/modules/Stream/styled.tsx Outdated Show resolved Hide resolved
src/browser/modules/DBMSInfo/DatabaseSelector.tsx Outdated Show resolved Hide resolved
@OskarDamkjaer
Copy link
Contributor Author

Merging as soon as the tests come back green

@jchen042 jchen042 merged commit efb87e8 into neo4j:master Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants