★ wanayoo — archive 1999 https://github.com/firefox-devtools/debugger/pull/8077Nouvelle recherche | Portail wanayoo
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

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

Signed-off-by: Ryan Duffy <ryan.duffy@lge.com>
@darkwing darkwing added the 📚 pr-wip label Mar 6, 2019
@darkwing
Copy link
Contributor

@darkwing darkwing commented Apr 10, 2019

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

@ryanjduffy
Copy link
Contributor Author

@ryanjduffy ryanjduffy commented Apr 14, 2019

I do! Thanks for the reminder, @darkwing!

ryanjduffy added 5 commits Apr 14, 2019
Signed-off-by: Ryan Duffy <ryan.duffy@lge.com>
Signed-off-by: Ryan Duffy <ryan.duffy@lge.com>
Signed-off-by: Ryan Duffy <ryan.duffy@lge.com>
@@ -130,6 +132,10 @@ async function expandFrames(
};

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

This comment has been minimized.

@ryanjduffy

ryanjduffy Apr 15, 2019
Author Contributor

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);

This comment has been minimized.

@ryanjduffy

ryanjduffy Apr 15, 2019
Author Contributor

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

This comment has been minimized.

@jasonLaster

jasonLaster Apr 15, 2019
Contributor

looks like a good catch

@ryanjduffy ryanjduffy marked this pull request as ready for review Apr 15, 2019
@ryanjduffy
Copy link
Contributor Author

@ryanjduffy ryanjduffy commented Apr 15, 2019

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

@jasonLaster
Copy link
Contributor

@jasonLaster jasonLaster commented Apr 15, 2019

quick scan. this looks really good.

Signed-off-by: Ryan Duffy <ryan.duffy@lge.com>
@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
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ryanjduffy ryanjduffy deleted the ryanjduffy:issue/7925 branch Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants