-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat(core): Add autoStart feature #98
base: main
Are you sure you want to change the base?
Conversation
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 is looking good so far! I left a few comments, but we need to know how this would work with multiple tours. Let me know if you want to pair on that 🙂
@@ -36,6 +36,8 @@ | |||
}, | |||
"dependencies": { | |||
"fast-equals": "^5.0.1", | |||
"object-hash": "^3.0.0", | |||
"react-native-mmkv-storage": "^0.9.1", |
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.
Let's move this to devDependencies as it's an optional peer dependency 🙂
@@ -73,6 +76,7 @@ | |||
"peerDependencies": { | |||
"react": ">=16.8.0", | |||
"react-native": ">=0.50.0", | |||
"react-native-mmkv-storage": ">=0.9.1", |
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 need to add this one to peerDependenciesMeta
and mark it as optional: true
const storage = new MMKVLoader().initialize(); | ||
|
||
export default storage; |
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 should try to avoid default exports in favor of named exports 🙂
const storage = new MMKVLoader().initialize(); | |
export default storage; | |
export const storage = new MMKVLoader().initialize(); |
@@ -0,0 +1,5 @@ | |||
import { MMKVLoader } from "react-native-mmkv-storage"; |
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 is an optional dependency, so it's possible that it's not present in some environments. Shouldn't the import fail in those cases? A way to solve that is by using dynamic imports.
import("react-native-mmkv-storage")
.then(({ MMKVLoader }) => {
// do something with the imported modules
})
.catch((error: unknown) => {
// handle whenever the module is not present
});
@@ -0,0 +1,5 @@ | |||
import { MMKVLoader } from "react-native-mmkv-storage"; | |||
|
|||
const storage = new MMKVLoader().initialize(); |
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.
Maybe we should initialize the store only when the autoStart
option is set to always | once
, otherwise it makes no sense to initialize something that is not going to be used 😉
} = props; | ||
|
||
const [current, setCurrent] = useState<number>(); | ||
const [spot, setSpot] = useState(ZERO_SPOT); | ||
const [tourId, setTourId] = useMMKVStorage("tourId", storage, ""); |
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.
renderStep(0); | ||
onStart?.(); |
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 these two can be replaced by calling the start()
function
|
||
const startOnce = useCallback(() => { | ||
if (!tourId) { | ||
setTourId(hash(steps)); |
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.
What happens if the user has multiple tour providers in their app. I think we will need them to provide an ID of the tour to use this feature, and the value we check against is the hash
of the steps. What do you think? 🤔
@@ -136,7 +138,18 @@ jest | |||
timing: timingMock, | |||
}, | |||
}; | |||
}); | |||
}) | |||
/* eslint-disable @typescript-eslint/no-unused-vars */ |
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 do we need this? 🤔
This PR address issue #50