Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Apply SourceMaps type to redux actions #8077

Merged
merged 8 commits into from
Apr 15, 2019

Conversation

ryanjduffy
Copy link
Contributor

@ryanjduffy ryanjduffy commented Mar 5, 2019

Fixes #7925

Summary of Changes

  • Change instances of source maps to use typeof SourceMaps instead of any/Object
  • Add a couple more types to address flow inference issue (see below)

Considerations

I couldn't find a way to import the typeof the default export of a module alongside named imports from the same module.

import typeof SourceMaps, { isGeneratedId } from "devtools-source-map";

Originally, I included two imports and suppressed the eslint error but changed when I realized the frequency this would be required. The current approach is to import the default export and use typeof in the instance typing later.

import typeof SourceMaps, { isGeneratedId } from "devtools-source-map";
// later
const sourceMaps: typeof SourceMaps = ...;

I also ran into a couple issues that seem to trace back to flow. I've documented them inline but noting them here for traceability in GitHub.

Explicitly typing an array wrapped by a Promise
facebook/flow#5294

Unable to correctly type the result of a spread on a union type.
facebook/flow#7298

@darkwing
Copy link
Contributor

Thank you for kick-starting this! Do you plan on continuing this PR?

@ryanjduffy
Copy link
Contributor Author

I do! Thanks for the reminder, @darkwing!

@@ -130,6 +132,10 @@ async function expandFrames(
};

originalFrames.forEach((originalFrame, j) => {
if (!originalFrame.location || !originalFrame.thread) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is the right approach. From what I read in XScope.search(), displayName will always be present and location may be present but thread shouldn't be. However, it was accessed below so perhaps I've misread.

@@ -94,7 +94,7 @@ export async function mapLocation(
return getGeneratedLocation(state, source, location, sourceMaps);
}

return sourceMaps.getOriginalLocation(location, source);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why source was passed here but seems invalid given the signature of getOriginalLocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like a good catch

@ryanjduffy ryanjduffy marked this pull request as ready for review April 15, 2019 04:56
@ryanjduffy
Copy link
Contributor Author

@jasonLaster + @darkwing - I think this is ready for review. Lemme know what you think.

@jasonLaster
Copy link
Contributor

quick scan. this looks really good.

Signed-off-by: Ryan Duffy <[email protected]>
@ryanjduffy ryanjduffy changed the title add type to one instance of sourceMaps Apply SourceMaps type to redux actions Apr 15, 2019
@jasonLaster jasonLaster merged commit d6ab717 into firefox-devtools:master Apr 15, 2019
@ryanjduffy ryanjduffy deleted the issue/7925 branch April 15, 2019 20:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[flow] type sourceMaps in actions
3 participants