symfony / stimulus-bridge

Stimulus integration bridge for Symfony projects
https://symfony.com/ux
75 stars 15 forks source link

Add typescript types #8

Closed chapterjason closed 3 years ago

chapterjason commented 3 years ago

Hope this is enough.

Tested with yarn link:

Screenshot 2020-12-11 at 9 37 21 PM

Fixes: #7

chapterjason commented 3 years ago

@tgalopin Yes I tested this with yarn link and with yarn pack.

Could be that the user needs to install @types/webpack-env, not sure what would be the best way, cause it will not be installed as a devDependency.

But I think it should be added as kind of documentation somewhere cause of the use from require.context it will say:

[tsl] ERROR in /[...]/assets/bootstrap.ts(5,45)
      TS2339: Property 'context' does not exist on type 'NodeRequire'.

With @types/webpack-env this error will not occur.

tgalopin commented 3 years ago

Wouldn't it make sense to provide a type package for stimulus-bridge that provide the types.d.ts file and include the webpack types?

chapterjason commented 3 years ago

@tgalopin You are right, not thought about it at all.

tgalopin commented 3 years ago

I think it's not worth creating an additional GitHub repo for this. What do you think of creating a types directory in this repository with a package.json inside and the types declaration? Would it make sense?

@weaverryan do you have an opinion about this?

chapterjason commented 3 years ago

@tgalopin sounds good, also thought about some type tests. https://github.com/Microsoft/dtslint

chapterjason commented 3 years ago

@tgalopin I took a look into the idea with a separate directory and testing.

Testing with dtslint too much overhead for this simple function that I would like to leave it out. A contributing documentation would be nice to mention that when changes are made the typing must be checked and adjusted.

For the dependency of the @types/webpack-env and own directory I took a look into the documentation (> dependencies).

All dependencies are managed by npm. Make sure all the declaration packages you depend on are marked appropriately in the "dependencies" section in your package.json. For example, imagine we authored a package that used Browserify and TypeScript.

Based on the documentation it would be okay to add the dependency, it has no disadvantages for non TypeScript users.

tgalopin commented 3 years ago

It has the disadvantage of including Webpack types on every machine without any use for non TS users. IMO the node_modules dir is already big enough, we should avoid adding direct dependencies for people who won't use it.

That's why I thought a types directory could be better: it would allow us to have a different module for both the bridge and Webpack types, allowing people to install it easily only when they need it. WDYT?

chapterjason commented 3 years ago

@tgalopin Any suggestion for the package name?

tgalopin commented 3 years ago

@symfony-types/stimulus-bridge ?

tgalopin commented 3 years ago

Thanks @chapterjason.

matteodem commented 3 years ago

Any hints when this will be deployed?

nspyke commented 3 years ago

For simplicity, I think this package should be under the @types namespace to ease autoloading. e.g. @types/symfony-stimulus-bridge seems more appropriate

https://www.staging-typescript.org/tsconfig#typeRoots

At the moment, I can't seem to get this to be imported. Would it be possible to update the Readme?