search

sg-wireless / pymakr-vsc

GNU General Public License v3.0
97 stars 25 forks source link

Unable to build / test preview according to docs #207

Closed Josverl closed 2 years ago

Josverl commented 2 years ago

What are the steps to reproduce this issue?

Tried to follow the steps in : https://github.com/pycom/pymakr-vsc/blob/next/CONTRIBUTE.md and I run into a few questions, and few errors, and i think a few missing dependencies Also fail to understand how the setup and packaging is supposed to work, as there appear to be some steps missing ( tsc)

I have stated to write my own 'getting started with dev' but do not want to overwrite the setup your are using without understanding.

A few of the open questions:

Development workstation:

Debugging

Testing

Transpiling

missing dev dependencies

npm scripts

Overall Project structure

I would suggest the following changes leading to the below structure: This uses the common structure used by .ts extensions , but also accommodates the functionality to read the package.json

I have managed to implement this in a fork , but am still working on getting the tests to work. see: https://github.com/Josverl/pymakr-vsc/tree/next-structure

Note-1: while the folder structure works , it is a bit funcky. this is a result of needing to include the package.json file as part of the source. This is needed due to the Pymakr.js:14 const manifest = require("../package.json"); that fails to compile unless package.json is included in the source tree for tsc. would it make sense to read the info from a different file than package.json ?

Note-2: After changing the structure the tests seem to be throwing additional errors, I assume they need to be updated for the path changes.

Proposed folder structure 

+---.devcontainer
+---.github
|   \---workflows
+---.vscode
+---.vscode-test
+---docs
+---example
|   \---workspace
|       +---multi
|       |   +---multi1
|       |   \---multi2
|       \---plain
+---media
+---node_modules
|   
+---src
|   +---commands
|   +---providers
|   +---stores
|   +---terminal
|   |   \---bin
|   +---test
|   |   +---suite
|   |   |   \---integration
|   |   |       \---file-management
|   |   |           \---_sample
|   |   |               +---ignoreme
|   |   |               \---includeme
|   |   +---utils
|   |   \---workspaces
|   |       +---e2e
|   |       \---integration
|   +---types
|   \---utils
|       \---specs
|           \---_sampleProject
+---out 
    \---src ( perhaps we can loose this level)
        +---commands
        +---providers
        +---stores
        +---terminal
        |   \---bin
        +---test
        |   +---suite
        |   |   \---integration
        |   |       \---file-management
        |   \---utils
        +---types
        \---utils
+---templates (may need to be included in out for packaging purposes)
|   +---empty
|   \---led-example
jakobrosenberg commented 2 years ago

Hi @Josverl, thanks for the feedback!

Should there be a device connected to the serial port ?

Yes. I think two devices are required currently. I'll add a note about this in CONTRIBUTE.md If possible, we should probably lower the number to 1.

works kind of: until I start making changes to a .ts file

There shouldn't be any .ts files, except for a global.d.ts file used for code completion in test files.

Transpiling

The source code doesn't need transpiling. It's already Javascript. Typescript is only by typedoc when generating API docs.

DevDependencies

The devDependencies should not be missing.

  "devDependencies": {
    "@semantic-release/changelog": "^6.0.1",
    "@semantic-release/git": "^10.0.1",
    "@types/glob": "^7.2.0",
    "@types/mocha": "^9.0.0",
    "@types/node": "16.x",
    "@types/vscode": "^1.63.1",
    "@vscode/test-electron": "^2.0.3",
    "eslint": "^8.1.0",
    "glob": "^7.1.7",
    "mocha": "^9.1.3",
    "probs": "^1.1.0-3",
    "semantic-release": "^19.0.2",
    "semantic-release-vsce": "^5.0.8",
    "typedoc": "^0.22.13",
    "typescript": "^4.6.3"
  },

We should maybe add vsce and nodemon. Especially nodemon as its used when testing. As for vsce, packaging happens in Github Actions and debugging doesn't require the package ( I think 🤔).

mctl

I think that was an old line used for debugging in the early stages. I've removed it now.

structure

For a classic TS project I would agree with the restructure.

Currently TS is achieved through inference and JSDoc to provide a best-of-both-worlds approach.

It provides:

I ❤️ the compilation free TS and I'm thrilled to see it catch on. Don't hesitate if you have any questions or concerns about it.

Josverl commented 2 years ago

Can you explain a bit more on how " TS is achieved through inference and JSDoc to provide a best-of-both-worlds approach."

While I'm by no means a ts or js expert, it appears that manually adding JSDoc tyoedefs to js has at least the same or more overhead, and less functunality than ts. But I guess that for existing. js that makes sense.

Josverl commented 2 years ago

Are you referring to https://itnext.io/microsoft-proposes-bringing-typescript-like-type-syntax-to-javascript-cdbbda7f5a2 ?

Josverl commented 2 years ago

I think adding a how to on vsce makes sense for contributers that do not have access to run the GHAction

jakobrosenberg commented 2 years ago

Typescript supports JSDoc, so instead of writing

const upload = ({ fsPath }: vscode.uri, device: import('../Device.js').Device} device, destination: string): Promise<any> => ...

We can write it in pure JS.

/**
* Uploads a file/folder to a device
* @param {vscode.Uri} uri
* @param {import('../Device.js').Device} device
* @param {string} destination not including /flash
*/
const upload = ({ fsPath }, device, destination) => ...

At a first glance, it may seem verbose in comparison, but once you look at the actual code, the JS function is far easier to read; It's types are neatly organized above and can feature commentary and examples. And lastly, the code requires no compilation.

For complex types, it's possible to use typedef files or .d.ts. Lately I've started migrating to .d.ts.

Back in the day Node was fairly helpless and needed help from tools like Babel for basic functionality. Adding TS didn't seem like an issue because we were already compiling our code. We were used to broken sourcemaps, broken buildtools, implicit OS requirements, convoluted debugging steps, version incompatibilities, esoteric configs and cargo cult solutions.

Node no longer needs support wheels and if types can be achieved without compilation, it's worth reconsidering the pros/cons of compilation; https://twitter.com/rich_harris/status/1323758415504646144

Josverl commented 2 years ago

thanks for the info , with the added understand I did another try, but keep running into a few errors.

false positive tsc errors

if there is a tsconfig.json, then this will show a lot of warnings for .js files that would get overwritten by tcs ( this is what led me on the tsc path in the first place as you explained that the tsc / tsconfig.sjon is only used by typedoc it seems that renaming that to doc_tsconfig.json should solve this

eslint errors and warnings

when I enable the eslint tasks There are 6 eslint errors and 50 warnings found. as this is still a preview, I assume this is still work in progress

Doc build

needs additional parameter --tsconfig tsconfig_doc.json probs v1.0.3 does not include types and results in an error in the doc build process this is solved by re-adding / updating the dependency that only leaves a bunch of warnings.

Ill create a PR for the changes and some added docs.

jakobrosenberg commented 2 years ago

Yes, I had to disable eslint, but will reenable it, once some of the kinks have been ironed out.

jakobrosenberg commented 2 years ago

:tada: This issue has been resolved in version 2.8.6 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

jakobrosenberg commented 2 years ago

Is it safe to close this / move it to discussions?