Skip to content

Improve development workflow#641

Merged
Ikuyadeu merged 15 commits intoREditorSupport:masterfrom
ManuelHentschel:workflow
May 29, 2021
Merged

Improve development workflow#641
Ikuyadeu merged 15 commits intoREditorSupport:masterfrom
ManuelHentschel:workflow

Conversation

@ManuelHentschel
Copy link
Copy Markdown
Member

@ManuelHentschel ManuelHentschel commented May 17, 2021

This PR makes working on the extension a bit easier.

By default, I disabled webpack and replaced the prelaunch task with a tsc watch task, which is considerably faster (especially on subsequent launches) than the webpack task. The script "scripts/enableWebpack.js" enables webPack again and can be called e.g. before publishing the extension. I also added some tsc watch tasks to compile the help/httpgd related typescript, but did not include these in the normal watch task, since most of the time they won't be changed anyways. Other possible modifications include

  • Adjusting the release actions to keep using webpack
  • Using yarn instead of npm. This basically comes down to replacing the lock file. In summary, yarn is compatible with the current package.json, but significantly faster. It's a bit less common than npm, apart from that I'm not aware of any drawbacks. For me personally, the switch would be worth it, but I also see the point that it might make running the extension from code a bit harder for new contributors.
  • Using subdirectories/other names for the typescript source files. Currently, there are some files (e.g. the help related stuff, or the upcoming liveshare files) that might as well be in their own folder. And I guess in most cases, we could remove the r from the beginning of file names, since they're all related to R anyways.

For the future, a slightly bigger change would be to set "strictNullChecks": true in tsconfig.json. This requires developers to work more cautiously when dealing with possibly undefined variables and requires rather many adjustments to the code base, since it cannot be turned off on a per-file basis (as opposed to eslint rules). But considering the rather strict rules implemented in .eslintrc.yml, I think it would make sense to also be strict here.

How to test:
Behavior of the installed extension shouldn't change.
When running the extension from code, startup should be significantly faster than with webpack.
When the package is published on Github (as build artifact, pre-release, release) or the vscode marketplace, webpack should be enabled again.

@ManuelHentschel ManuelHentschel changed the title WIP: Improve development workflow Improve development workflow May 20, 2021
@ManuelHentschel ManuelHentschel marked this pull request as ready for review May 20, 2021 22:46
@ManuelHentschel
Copy link
Copy Markdown
Member Author

ManuelHentschel commented May 20, 2021

I guess using yarn and organizing files in subdirectories is optional anyways, so I wouldn't mind skipping it here.

@ManuelHentschel ManuelHentschel requested review from Ikuyadeu and renkun-ken and removed request for Ikuyadeu May 20, 2021 22:48
@Ikuyadeu
Copy link
Copy Markdown
Member

We used yarn for a while, but I think I switched back to npm due to compatibility issues with webpack or GitHub Action.
If yarn is available, I think it's best.

@ManuelHentschel
Copy link
Copy Markdown
Member Author

We used yarn for a while, but I think I switched back to npm due to compatibility issues with webpack or GitHub Action.
If yarn is available, I think it's best.

Using yarn instead of npm seems to work fine, both with and without webpack. If you find any problems with it, let me know.

@Ikuyadeu
Copy link
Copy Markdown
Member

Using yarn instead of npm seems to work fine, both with and without webpack. If you find any problems with it, let me know.

OK, changing that is not so large work. And we can check bugs on this PR.

Using subdirectories/other names for the typescript source files. Currently, there are some files (e.g. the help related stuff, or the upcoming liveshare files) that might as well be in their own folder. And I guess in most cases, we could remove the r from the beginning of file names, since they're all related to R anyways.

It sounds great, and I think, we can put them in one folder such as help, http.

@ManuelHentschel
Copy link
Copy Markdown
Member Author

I moved help and plot related code to their own folders, and using yarn over npm seems to work fine with Github actions/vsce

@Ikuyadeu
Copy link
Copy Markdown
Member

LGTM, Thank you.
I'll review more detail this week.

BTW, for release process, I am planning to use a draft release instead of the latest, that do not need to update the tag index.
https://github.com/release-drafter/release-drafter

@Ikuyadeu
Copy link
Copy Markdown
Member

@ManuelHentschel
This point was that I should have reviewed before this PR, but src / helpViewer / index.ts and the other helpviewer modules are interdependent.
This design complicates the modifications when editing each other.
In my idea, one solution is to divide the index.ts functions into other files such as util.ts.

I want to merge this PR once and solve this problem with a new PR, is that okay? Or do you want to do it here?

@Ikuyadeu
Copy link
Copy Markdown
Member

For other parts, I think we can merge them.

@ManuelHentschel
Copy link
Copy Markdown
Member Author

I want to merge this PR once and solve this problem with a new PR, is that okay? Or do you want to do it here?

I agree that it's best to address that issue in a different PR and merge this PR for now.

@Ikuyadeu Ikuyadeu merged commit 8787b69 into REditorSupport:master May 29, 2021
@ManuelHentschel ManuelHentschel deleted the workflow branch May 31, 2021 08:45
renkun-ken pushed a commit that referenced this pull request Jun 11, 2021
* Liveshare Functionality

Implements session watcher features for LiveShare guests

* Update package-lock.json

* Review changes (1)

- isLiveShare now relies on liveSession instead of a new vsls.getApi() call
-- Many functions are now synchronous as a result
- Various tooltip changes to clarify 'forward guest commands'
- Clean up helper bool functions

* Move files to liveshare folder

As per #641, liveshare files are moved to a /liveshare/ folder, and the "r" prefix has been removed from their name

* Review changes (2)

- Liveshare API handling
-- Abort on timeout
-- Retry button
- Guest & host workspace always in sync
- Guests no longer create files, instead relying on webview + virtual docs
- Fix bug with webviews returning object instead of string

* Resolve upstream changes (#2)

* Share httpgd session with guest

- Httpgd sessions are shared with guests through vsls shareBrowser functionality
- Resolve some conflicts from upstream merge

* Fix merge error + refresh tree

- Remove duplicate function declaration
- Refresh liveshare tree on retry api

* Review changes (3)

- Use liveShare.defaults as per #620
- Change overuse of brackets

* Better integrate httpgd

- The shared httpgd is now opened/reopened even if closed
- Shared httpgd is now shared even if started before liveshare
- Minor bug fixes

* *Better* integrate httpgd

- Instead of doing a workaround browser solution,  use the native httpgd webview

* Minor changes

- Change timeout default: 3000 => 10000
- Remove leftover information message
- Fix accidental merge overwrite

* Fix guest httpgd eagerness

- notifyGuestPlotManager is now called less often, preventing it from taking focus unexpectedly

* Minor changes

- .UUID -> uuid
- Shorten commandNode label

* Review changes (4)

- request() to liveShareRequest()
- onRequest() to liveShareOnRequest()
- use index design pattern
- remove global shareurl
- remove badge from README

* Lint + misc change

- Appease the linter gods
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.

2 participants