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

refactor: remove SourceMap dependency from dep-graph #1908

Merged
merged 14 commits into from
Jul 13, 2021

Conversation

dsherret
Copy link
Contributor

@dsherret dsherret commented Jul 8, 2021

There isn't any need for analyze_dependencies to use a SourceMap. Instead it can stay working with span/byte pos units so it's the same as the AST (that way, we don't mix units).

Additionally, it doesn't need to be limited to SingleThreadedComments and can instead take anything that implements the Comments trait.

What I'm trying to do is hold onto swc ASTs in Deno's language server, but I need everything to be Send in order to do that. Source map doesn't allow that, so it's up on the chopping block right now. Next will be SourceFile and SingleThreadedComments.

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Btw, Lrc<SourceMap> implements Send + Sync if you enable concurrent feature of swc_common.

ecmascript/dep-graph/src/lib.rs Outdated Show resolved Hide resolved
@dsherret
Copy link
Contributor Author

dsherret commented Jul 9, 2021

Btw, Lrc implements Send + Sync if you enable concurrent feature of swc_common.

Interesting. I feel like I knew that, but forgot and that's good to know! I don't want to turn on that feature because it would be a global change, would most likely be slower in our case, and introduces the possibility of a new class of bugs. I think I will be able to manage by only holding onto things like SourceFile and SourceMaps structs when we need them for things like transpiling and bundling or perhaps creating those after the fact again when needed. The rest of it should ideally work via only holding onto the AST, source file text, and comments (I will take them out of the SingleThreadedComments).

@kdy1
Copy link
Member

kdy1 commented Jul 9, 2021

I see. I'm okay with current changes, but I'm not sure if you want me to merge it.

@dsherret
Copy link
Contributor Author

dsherret commented Jul 9, 2021

@kdy1 yeah, not yet. I want to look into fixing this possible race condition. I'm just looking into your comments.

@dsherret dsherret mentioned this pull request Jul 9, 2021
7 tasks
@kdy1
Copy link
Member

kdy1 commented Jul 13, 2021

Is this PR ready? If not, please ping me when you are done :)

@dsherret
Copy link
Contributor Author

Hey @kdy1, yeah, it is now. I’m just not sure about the changes to the comments trait and what would be best there.

@kdy1 kdy1 merged commit 6dc6d8a into swc-project:master Jul 13, 2021
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants