Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update template files #8

Merged
merged 5 commits into from
Mar 15, 2024
Merged

Update template files #8

merged 5 commits into from
Mar 15, 2024

Conversation

iowillhoit
Copy link
Contributor

@iowillhoit iowillhoit commented Mar 13, 2024

Updates template files to be in sync with current standards.
The source-deploy-retrieve and source-tracking repos were used as a reference.

See QA notes below.
Test with: salesforcecli/plugin-dev#492

@W-15235736@

@iowillhoit
Copy link
Contributor Author

Some QA Notes:

Copy link
Collaborator

@mdonnalley mdonnalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some non-blocking questions/comments

tsconfig.json Outdated
@@ -1,7 +1,10 @@
{
"extends": "@salesforce/dev-config/tsconfig-strict",
"compilerOptions": {
"outDir": "./lib"
"outDir": "./lib",
"skipLibCheck": true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? Would be nice to avoid it if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both STL and SDR have this set. Within the last year, Shane suggested I turned this on in a different plugin that was having issues with glob types. How about I comment this out so folks can easily reenable this if they need it? The template commpiles fine without it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies to the graceful-fs question too.

Given that this is an internal-only template that's not widely used (if at all) I would argue that there's not much reason to spoon feed libraries and compiler options to people. I'd rather keep it as bare-bones as possible and trust that whoever is using the library understands what they're doing.

I think my concern is that things like this don't ever get removed because they came with the template. So with skipLibCheck you're opening the potential for type errors at runtime.

But like I said, this isn't widely used so it probably doesn't matter too much in the long run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's perfectly fair. I'll remove both of these things 👍 Thanks for the review

* All rights reserved.
* Licensed under the BSD 3-Clause license.
* For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/

import { Messages, Logger } from '@salesforce/core';

Messages.importMessagesDirectory(__dirname);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing __dirname made me wonder if we should convert the package to ESM? Or maybe have one template for both?? Or maybe just keep it CJS until it becomes an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait for now. We can migrate it when we migrate STL and SDR

package.json Outdated
"@salesforce/core": "^6.7.0",
"@salesforce/kit": "^3.0.15",
"@salesforce/ts-types": "^2.0.9",
"graceful-fs": "^4.2.11"
Copy link
Collaborator

@mdonnalley mdonnalley Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is graceful-fs used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not, but it is used in both Source Tracking and Source Deploy Retrieve. Fs operations will be common in libraries, I thought I would leave it as a suggestion to take advantage of.

@iowillhoit iowillhoit merged commit 98d1b72 into main Mar 15, 2024
8 checks passed
@mdonnalley mdonnalley deleted the ew/update-template branch March 15, 2024 19:23
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