60 resource viewer v2 #380
Conversation
There was a problem hiding this comment.
Thanks for the hard work on this!! So close!
Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @katherinejensen00)
.vscode/settings.json line 66 at r1 (raw file):
} ], "cSpell.words": ["usfm"]
We already have a cspell configuration in paranext-core/cspell.json that is picking up usfm. Is yours not respecting this file? Do you have some overrides in your VSCode settings that prevent you from using this file?
extensions/src/resource-viewer/resource-viewer.ts line 17 at r1 (raw file):
const unsubscribers: UnsubscriberAsync[] = []; class ResourceDataProviderEngine
Is this used? Can you remove it?
extensions/src/resource-viewer/resource-viewer.ts line 130 at r1 (raw file):
const unsubPromises = [ papi.commands.registerCommand('resourceViewer.getUsfm', () => {
Is this used? Can you remove it?
extensions/src/resource-viewer/resource-viewer.ts line 141 at r1 (raw file):
papi.webViews.getWebView(resourceWebViewType, undefined, { existingId: '?' }); if (resourceDataProvider) {
This seems to be unnecessary test code - can we remove it?
extensions/src/resource-viewer/resource-viewer.ts line 161 at r1 (raw file):
); logger.info('The Resource Viewer is finished activating!'); return combinedUnsubscriber;
The way to register unsubscribers has changed. Please refer to https://discord.com/channels/1064938364597436416/1136698231250755664 for how to update :) thanks!
extensions/src/resource-viewer/resource-viewer.d.ts line 7 at r1 (raw file):
// TODO: Should this eventually be turned into ScrText or should it utilize ScrText? export type ResourceDataTypes = {
Is this used? Can you remove it?
extensions/src/resource-viewer/resource-viewer.d.ts line 28 at r1 (raw file):
* number type is sneezeId and will update an existing sneeze */ export type ResourceDataProvider = IDataProvider<ResourceDataTypes>;
Is this used? Can you remove it?
extensions/src/resource-viewer/resource-viewer.d.ts line 31 at r1 (raw file):
declare module 'papi-commands' { export interface CommandHandlers {
Are these used? Can you remove them?
extensions/src/resource-viewer/resource-viewer.web-view.tsx line 122 at r1 (raw file):
/** * Scripture text panel that inserts the chapter string into innerHTML or a contentEditable div.
This comment appears to be incorrect (I think it was incorrect in the POC as well... Oops. Sorry)
extensions/src/resource-viewer/resource-viewer.web-view.tsx line 130 at r1 (raw file):
// eslint-disable-next-line jsx-a11y/no-static-element-interactions <div className="text-panel"> <RefSelector
If you move this RefSelector into ResourceViewer, add a useState for its scrRef, and use that state in the useData.ChapterUsx hook, you'll have the scrRef hooked up to the usx editor :)
extensions/src/resource-viewer/resource-viewer.web-view.tsx line 159 at r1 (raw file):
'usfm', useMemo(() => new VerseRef('GEN', '1', '1', ScrVers.English), []), 'Loading Psalm 1...',
This loading statement might be a bit too specific :)
There was a problem hiding this comment.
Thanks for all of your help, TJ!
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @tjcouch-sil)
.vscode/settings.json line 66 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
We already have a cspell configuration in
paranext-core/cspell.jsonthat is picking up usfm. Is yours not respecting this file? Do you have some overrides in your VSCode settings that prevent you from using this file?
Done.
extensions/src/resource-viewer/resource-viewer.ts line 17 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Is this used? Can you remove it?
Done.
extensions/src/resource-viewer/resource-viewer.ts line 130 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Is this used? Can you remove it?
Done.
extensions/src/resource-viewer/resource-viewer.ts line 141 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
This seems to be unnecessary test code - can we remove it?
Done.
extensions/src/resource-viewer/resource-viewer.ts line 161 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
The way to register unsubscribers has changed. Please refer to https://discord.com/channels/1064938364597436416/1136698231250755664 for how to update :) thanks!
This ended up not being needed, so I just removed it.
extensions/src/resource-viewer/resource-viewer.d.ts line 7 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Is this used? Can you remove it?
Done.
extensions/src/resource-viewer/resource-viewer.d.ts line 28 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Is this used? Can you remove it?
Done.
extensions/src/resource-viewer/resource-viewer.d.ts line 31 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Are these used? Can you remove them?
Done.
extensions/src/resource-viewer/resource-viewer.web-view.tsx line 122 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
This comment appears to be incorrect (I think it was incorrect in the POC as well... Oops. Sorry)
Done.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewed 6 of 11 files at r1, 6 of 6 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @katherinejensen00)
extensions/src/resource-viewer/package.json line 13 at r3 (raw file):
"license": "MIT", "devDependencies": { "usxeditor": "^0.0.9-alpha"
This is definitely not a devDependency since you use the package in production code. Change this to dependencies.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)
extensions/src/resource-viewer/package.json line 13 at r3 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
This is definitely not a
devDependencysince you use the package in production code. Change this todependencies.
Since it is bundled into the extension main code, I would say it is a devDependency. It is not necessary for npm to install it when you're using the pre-built extension code.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)
extensions/src/resource-viewer/package.json line 13 at r3 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Since it is bundled into the extension main code, I would say it is a
devDependency. It is not necessary for npm to install it when you're using the pre-built extension code.
I saw Ira's comment and committed the change. I was about to push the change when I saw TJ's comment. Should I revert that commit?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)
extensions/src/resource-viewer/package.json line 13 at r3 (raw file):
Previously, katherinejensen00 wrote…
I saw Ira's comment and committed the change. I was about to push the change when I saw TJ's comment. Should I revert that commit?
Let's wait for Ira.
|
Previously, tjcouch-sil (TJ Couch) wrote…
According to the npm definition and since it's used in production it belongs in |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)
extensions/src/resource-viewer/package.json line 13 at r3 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
According to the npm definition and since it's used in production it belongs in
dependencies(it should only go indevDependenciesif it is required only in your development phase). @tjcouch-sil is correct but only relative to the main app, however the localpackage.jsonfile should always describe things for itself not how they are for a consumer.
I disagree that the npm definition indicates this should be a dependency because we are only depending on the usxeditor package existing in our environment in local development. I believe the difference in the way npm treats the two backs up my position. A dependency is installed when you install a package through npm as a dependency (or dev dependency) of your project, whereas a devDependency is not installed when you install a package through npm as a dependency (or dev dependency) of your project. If I install a library as a dependency of my app, I don't want to install that package's devDependencies because I am just using the final built files of that library. However, I do want to install that package's dependencies because the nature of dependencies is that they must exist in order for the library to work.
In my perspective, putting a package in dependencies is essentially equivalent to marking that package as external in webpack. ERB sort-of makes this connection explicit by pulling dependencies from one of its package.json files and marking them external programmatically. However, I concede that they only did that with the package.json relating to native modules, so they're not hard on this issue (probably because this topic is not well understood and is debated widely). My perspective is that dependencies approximately equals external because marking something external means webpack will leave the import statements so node actually looks for that module and actually imports it instead of bundling it into your code, thereby making it a literal dependency in your production code.
We are distributing bundled extension code that has no dependencies other than those marked external in our webpack config. Let's pretend extensions were all on npm. If someone installed our extension as a library, they would have our built code. The only things they would want to install are the dependencies of our project because those are needed to run our code. devDependencies (like usxeditor) are already bundled into the extension bundle, so we don't want to install devDependencies again.
The difference is if you need it in production vs local development, so we can think about what code is being run. If we're developing locally, we're running webpack and bundling a bunch of dependencies together to make the main file that gets run. Hence we mark many things like webpack and usxeditor as devDependencies. If it's already built for production, we don't need those devDependencies because Platform.Bible is running the main file that's already built and ready to go.
The difference between dependencies and devDependencies in terms of bundled libraries appears to be debated fairly widely across the internet. A quick Google search proves this to be true - there are many people making very confident assertions that they are right on both sides as usual in the web dev world ;) I came down on the side that I did because it follows the way npm actually uses dependencies and devDependencies; there seem to be incongruences in the meanings of dependencies and devDependencies in my mind otherwise; and it seems there is not much consensus in the web dev world to the contrary.
The following comment from some random guy seems to represent my position well: webpack/webpack#520 (comment)
|
Thanks @tjcouch-sil for that great explaination so lets go with that. In your first link it talked about bundledDependencies which I don' think I knew about before. I'm wondering if we should investigate using that to make situations like this clearer? |
There was a problem hiding this comment.
Good question; I'm not really exactly sure what bundledDependencies does, but it seems unrelated to webpack-related bundling. Sounds like maybe npm will bundle those dependencies into the final deliverable, which is different than webpack bundling, which embeds the dependency code into your built files.
We should probably put something about how we use dependencies vs devDependencies somewhere in our documentation - does the style guide seem like a good place for it? I kinda feel like maybe a different place would be more suitable. Maybe we could add a quick description line in the style guide and link to a separate wiki page explaining more in-depth?
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)
|
Would the template README be the right place? Otherwise go with your idea. |
There was a problem hiding this comment.
I'd prefer to put it somewhere that is more scoped to our code. Since this is more of a preference than a semantically meaningful difference specifically for extensions because of the way we load them, I'd say it doesn't fit too well in the template README. I added a bullet to the style guide at the bottom of the list "Specify bundled dependencies in devDependencies". Let me know what you think. I'm open to moving it somewhere else if you have other thoughts :)
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)
|
|
Here is the code for a super basic resource viewer using Tom Hindle's editor that he started. Note, the reference control will be hooked up to the resource viewer in another task and is just there for now as a reference ;).
This change is