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

feat(debugger): Update filewatch to look for .map and .js and send /reload to MC #248

Merged
merged 8 commits into from
Oct 29, 2024

Conversation

chmeyer-ms
Copy link
Contributor

@chmeyer-ms chmeyer-ms commented Oct 25, 2024

Similar to this PR here but leans into VSCode settings for added flexibility. By using VSCode settings, users can choose to save settings globally or per workspace and it also gives us a place for documentation.

Thanks for the general glob pattern idea Dingsel. Will this solve your use cases?

  • file watching is off by default
  • if enabled then both source map clear and /reload happen on source changes.
  • if only localRoot is defined (no sourceMapRoot), the file watch pattern is *.js files from localRoot
  • if sourceMapRoot is defined, the file watch pattern is for *.js and *.map from sourceMapRoot
  • if globPattern is specified, the root is the workspace.
  • delay for sending /reload to MC, minimum is 100ms to debounce the file changes.

Added a button to the home panel to open Settings UI directly to the debugger page.

Addresses issue #130

@chmeyer-ms chmeyer-ms self-assigned this Oct 25, 2024
src/Session.ts Outdated
this._sourceMaps.reset();
});

const minReloadDelay = 500;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels weird to me to data drive this but to also have a minimum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

note: We should probably make sure it's not negative :D

@@ -22,6 +22,10 @@ const onShowDiagnosticsPanel = () => {
vscode.postMessage({ type: 'show-diagnostics' });
};

const onShowSettings = () => {
vscode.postMessage({ type: 'show-settings' });
Copy link
Collaborator

Choose a reason for hiding this comment

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

One of these days we should look up how to make these events more type-safe/shared strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Only real way is if we publish a package that specifies these, and we generate them from some metadata we spit out from the game. IMO, not worth the effort.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we converted this to a workspace we could use a shared (unpublished) package right?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I think I misunderstood initially a goal to share with the debugger protocol, but this is between the extension and the webview-ui. Yes you are correct, we can solve it with an intermediate package that way.

src/Session.ts Outdated Show resolved Hide resolved
src/Session.ts Outdated Show resolved Hide resolved
Comment on lines +1061 to +1067
const debounce = (func: () => void, wait: number): (() => void) => {
let timeout: NodeJS.Timeout;
return () => {
clearTimeout(timeout);
timeout = setTimeout(func, wait);
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: extra to helper function?

chmeyer-ms and others added 2 commits October 29, 2024 11:13
Co-authored-by: Raphael Landaverde <[email protected]>
Co-authored-by: Raphael Landaverde <[email protected]>
@chmeyer-ms chmeyer-ms merged commit e63585a into main Oct 29, 2024
2 checks passed
@chmeyer-ms chmeyer-ms deleted the chmeyer/on-reload branch October 29, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants