search

imodeljs / itwin-viewer

Monorepo that contains the iTwin Viewer npm package and its related packages
MIT License
15 stars 13 forks source link

Random cleanup #9

Closed calebmshafer closed 4 years ago

calebmshafer commented 4 years ago
calebmshafer commented 4 years ago

@kckst8 I have a some questions...

  1. I'm not sure we should be delivering the extension as part of the sample. Do you have any objections to pulling one from the Extension Service?
  2. Do we want to have all users of the package using the iTwinViewer GPRId?
  3. AppInsights is fairly heavy to require all users of the viewer to bring, especially if they never are going to use it... Could we make that a configuration instead somehow?
  4. Can you change the ItwinViewer to ITwinViewer? That's how we handle all of the iModel based names, IModelDb.
  5. Did you plan to setup lint-staged and some of those other tools? I notice they're still in the package.json
  6. Can we append -react to the @bentley/itwin-error-handling package? This is the standard we've been doing in the viewer-components repo for all packages.
  7. One last one :), any reason to have 2 levels of folder nesting before the contents? Instead of packages/apps and packages/modules just have apps and modules?
kckst8 commented 4 years ago

@kckst8 I have a some questions...

  1. I'm not sure we should be delivering the extension as part of the sample. Do you have any objections to pulling one from the Extension Service? - That would actually be ideal, but we would need one that supports UI 2.0. I was not aware of one in the extension service. If there is one, please let me know and I will update

  2. Do we want to have all users of the package using the iTwinViewer GPRId? - It's a good question. We do want usage logging for the viewer. I'll have to ask about this. Maybe we can add an optional parameter and default to the viewer

  3. AppInsights is fairly heavy to require all users of the viewer to bring, especially if they never are going to use it... Could we make that a configuration instead somehow? - It is configurable. The app insights key is optional. The HOC is still used but will not log anything if no key is provided (in my testing). It's an important tool since we have been asked to track usage time within the viewer.

  4. Can you change the ItwinViewer to ITwinViewer? That's how we handle all of the iModel based names, IModelDb. - If you mean the class, this was intentional since it violates default linting rules (classes must be pascal-cased). We can update our rules if you think it's a large issue. Otherwise I just assume follow the most commonly used standards? I guess it depends on if you feel I and Twin are mutually exclusive. At any rate, it would require an eslint exception (which is fine if needed)

  5. Did you plan to setup lint-staged and some of those other tools? I notice they're still in the package.json - Yes. It's next on the TODO list so I left some configuration in place. It shouldn't hold up going public, however.

  6. Can we append -react to the @bentley/itwin-error-handling package? This is the standard we've been doing in the viewer-components repo for all packages. - Yes. This makes sense.

  7. One last one :), any reason to have 2 levels of folder nesting before the contents? Instead of packages/apps and packages/modules just have apps and modules? - This is a pretty common monorepo structure. Admittedly, it's probably most likely for lerna/yarn workspaces repos where providing a single location for workspaces is preferable. Still, I like keeping the root cleaner and this is a clear indication of where to find packages, while keeping the subtypes of packages distinct as well. I'm not opposed to changing it if it is an issue.

calebmshafer commented 4 years ago

@kckst8

  1. Sounds good. I'll work on that this upcoming week. 4) Possibly not worth all the effort but iModel.js will be doing that with our new ESLint configuration (hopefully released soon)

  2. Up to you, your repo :)