Add SLDS Linter #101

engai · 2017-09-07T18:30:52Z

What does this PR do?

Adds SLDS Linter feature to Lightning Package

What issues does this PR fix or reference?

@W-4267770@

vazexqi · 2017-09-07T18:35:18Z

packages/salesforcedx-vscode-lightning/README.md

@@ -1,5 +1,5 @@
 # salesforcedx-vscode-lightning


@ruthemmanuelle - Could you review this README for @ayeshakmaz and @engai?

vazexqi · 2017-09-07T20:12:48Z

@engai @ayeshakmaz Could you take a look at your test to see why it's failing on windows? Could it be you are looking for a particular path and that you are using / instead of \?

engai · 2017-09-07T20:33:06Z

@vazexqi Yes. It's trying to open ./assets/sfdx-simple. How would you suggest fixing this to work on Mac, Linux & Windows?

vazexqi · 2017-09-07T21:17:10Z

Try using https://nodejs.org/api/path.html#path_path_joint paths. You might want to also check to see that you are not using / in your code and not just the tests.

codecov · 2017-09-08T23:26:17Z

Codecov Report

Merging #101 into develop will decrease coverage by 0.07%.
The diff coverage is 73.33%.

@@             Coverage Diff             @@
##           develop     #101      +/-   ##
===========================================
- Coverage    82.66%   82.58%   -0.08%     
===========================================
  Files           66       67       +1     
  Lines         1817     1832      +15     
  Branches       228      229       +1     
===========================================
+ Hits          1502     1513      +11     
- Misses         279      283       +4     
  Partials        36       36

Impacted Files	Coverage Δ
...ackages/salesforcedx-vscode-lightning/src/index.ts	`73.33% <73.33%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9a74ec...59f7bd4. Read the comment docs.

codecov · 2017-09-08T23:26:17Z

Codecov Report

Merging #101 into develop will increase coverage by 2.2%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop     #101     +/-   ##
==========================================
+ Coverage    82.47%   84.67%   +2.2%     
==========================================
  Files           65       27     -38     
  Lines         1723      855    -868     
  Branches       219      129     -90     
==========================================
- Hits          1421      724    -697     
+ Misses         265       97    -168     
+ Partials        37       34      -3

Impacted Files	Coverage Δ
...s/salesforcedx-apex-debugger/src/messages/index.ts	`41.66% <0%> (-51.2%)`	⬇️
...rcedx-vscode-core/src/commands/forceApexTestRun.ts
...sforcedx-vscode-core/src/commands/forceTaskStop.ts
...ode-core/src/commands/forceLightningEventCreate.ts
...core/src/commands/forceLightningComponentCreate.ts
...ges/salesforcedx-vscode-core/src/messages/index.ts
...orcedx-vscode-core/src/commands/forceSourcePull.ts
...cedx-vscode-core/src/commands/forceAuthWebLogin.ts
...ges/salesforcedx-vscode-apex/src/messages/index.ts
...vscode-core/src/predicates/salesforcePredicates.ts
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5757f0...5d090c4. Read the comment docs.

engai · 2017-09-11T14:43:53Z

Paths look correct at the moment (some are glob patterns). The extension and tests run when tested in vscode on windows. However, they aren't running with the test script on Windows. Any ideas on what might be happening here?

vazexqi · 2017-09-11T15:04:56Z

@engai - Could you get a Windows VM and run the tests locally or add more logging for when the test fails?

The error is

lerna ERR! execute   1) SLDS Deprecated Class Name Should create deprecatedClassName command:
lerna ERR! execute      AssertionError: expected [ Array(693) ] to include 'deprecatedClassName'
lerna ERR! execute       at Object.<anonymous> (C:\projects\salesforcedx-vscode\packages\salesforcedx-slds-linter\test\deprecatedClassName.test.ts:27:28)
lerna ERR! execute       at Generator.next (<anonymous>)
lerna ERR! execute       at fulfilled (C:\projects\salesforcedx-vscode\packages\salesforcedx-slds-linter\out\test\deprecatedClassName.test.js:10:58)
lerna ERR! execute       at <anonymous>
lerna ERR! execute 
lerna ERR! execute 

``` but it's not clear why it can't find what it's looking for.

engai · 2017-09-11T17:18:15Z

@vazexqi the test is essentially looking for our command within the workspace command list. This will only be there when the extension runs. Not sure what else to log in this case. The extension functions and tests pass when ran in VSCode on our VM, but the extension doesn't seem to run when ran from the test script.

The correct workspace is opened, but no syntax highlighting or linting appears.

vazexqi · 2017-09-11T18:31:37Z

packages/salesforcedx-slds-linter/test/deprecatedClassName.test.ts

+
+  it('Should create deprecatedClassName command', async () => {
+    const commandList = await vscode.commands.getCommands(true);
+    expect(commandList).to.include('deprecatedClassName');


@engai I guess you can log here to see what is the list of commandList on the autobuild and how that differs from the one you have locally. I am not yet sure why the test will fail since this seems to be a simple test.

engai · 2017-09-11T21:43:06Z

When working on mac and within vscode on windows, it's logging 45 more commands. These include ours and commands from git and sfdx:force. When running on the test script however, it's not logging these commands.

vazexqi · 2017-09-11T22:56:58Z

@engai That is helpful. So we can at least narrow it down to the command not loading.

How are you running the tests locally? How does it differ from the appveyor.yml and travis.yml files? Could you be running it from a different directory?

On that note, I just noticed that your command doesn't follow the convention. Could you rename your command to sfdx.force.lightning.slds.deprecated.class.fix (or something similar)? We need to have some form of a namespace to ensure that command names do not collide and just deprecatedClassName is too generic.

engai · 2017-09-11T23:13:40Z

@vazexqi To run via test script (Does not pass) - Following these steps in the salesforce-vscode directory:

npm run bootstrap
npm run compile
npm run test (Sometimes ran from /packages/salesforce-slds-linter to ignore the other packages' tests)

Running in VSCode (Passes):

Run Task > Bootstrap if changing anything in dependencies
Launch Salesforce DX VS Code SLDS Linter Tests to run tests in ext. dev host

On a side note, currently unable to run tests via the script (was working yesterday). Seems like the script can't find a VSCode executable:

Have you run into this problem? Looks similar to microsoft/vscode-extension-vscode#30

As for the command name, thanks for the heads up. I'll go ahead and change that.

vazexqi · 2017-09-11T23:16:06Z

I just ran the tests locally and I'm able to produce the issue (on my Mac). I suspect that it's not loading something when run from the top-level.

git clone git://github.com/forcedotcom/salesforcedx-vscode.git
cd salesforcedx-vscode
npm install
npm run compile
npm run test:without-system-tests (we don't need to run the UI tests to repro the issue)

vazexqi · 2017-09-11T23:50:35Z

packages/salesforcedx-vscode-lightning/test/deprecatedClassName.test.ts

-      await vscode.workspace.openTextDocument(res[0]).then(
-        document => vscode.window.showTextDocument(document)
+      res = await vscode.workspace.findFiles(
+        path.join('**', 'DemoComponent.cmp')


You should be precise and look exactly for the .cmp you know has the deprecatedclassnames.

vazexqi · 2017-09-11T23:53:58Z

@engai - Take a look at my latest commit where I moved your tests to the salesforcedx-vscode-lightning package. The tests need to be there since it depends on an actual activation event.

It runs when you do it from VS Code itself since you are loading all the packages (see .vscode/launch.json). But, when you run the tests from the command line, the tests only load the extensions for that package.

engai · 2017-09-12T00:07:55Z

Ah that makes sense. The tests aren't passing within VSCode, but i'll work on that within the lightning tests

vazexqi

Also, don't forget to change the command name to something more descriptive.

vazexqi · 2017-09-12T00:15:46Z

.vscode/launch.json

+      "preLaunchTask": "Compile"
+    },
+    {
+      "name": "Launch Salesforce DX VS Code SLDS Linter Tests",


@engai - This one probably needs to be updated since I removed the ${workspaceRoot}/packages/salesforcedx-slds-linter/assets/sfdx-simple

vazexqi · 2017-09-12T00:16:55Z

packages/salesforcedx-vscode-lightning/test/deprecatedClassName.test.ts

+      res = await vscode.workspace.findFiles(
+        path.join('**', 'DemoComponent.cmp')
+      );
+      await vscode.workspace


@engai Since you open the file, you should probably have an after that closes it too (just to be tidy). So far it's not causing problems but as we have more tests in the future, I think it's good for each test to clean up after itself.

Added an after to close the editor. Looks like closeFile is deprecated.

engai · 2017-09-12T00:58:16Z

@vazexqi Wasn't sure if it would affect the end-to-end tests, i'll remove mine and add the .cmp file to system-test.

Though .DS_Store was in the .git_ignore, i'll remove those as well.

vazexqi · 2017-09-12T01:12:03Z

In one of my previous commits I already added the DemoComponent to the system-tests/assets folder. You should be able to see it.

ayeshakmaz · 2017-09-12T01:23:47Z

@vazexqi I'm looking in system-tests/assets, but not seeing that moved DemoComponent

vazexqi · 2017-09-12T01:39:55Z

@ayeshakmaz - Crap. I must have forgotten to commit it. One moment.

vazexqi · 2017-09-12T02:01:18Z

@ayeshakmaz and @engai - I've added the DemoComponent.cmp now.

vazexqi · 2017-09-12T02:03:29Z

packages/salesforcedx-slds-linter/src/messages/i18n.ts

+ *
+ * If ommitted, we will assume _message.
+ */
+export const messages = {


@ruthemmanuelle - Could you take a look at these messages? There is a screenshot that shows where there are used in this PR.

vazexqi · 2017-09-12T14:29:37Z

@Sweetman - Here's another case where the travis builds just hangs.

engai · 2017-09-12T23:35:11Z

@vazexqi I've added 3 tests to our slds-linter package. Let us know if there is anything else we should add. Appveyor build looks good as well.

vazexqi · 2017-09-13T00:08:43Z

packages/salesforcedx-slds-linter/test/server.test.ts

+  it('Should send correct string', () => {
+
+    validateTextDocument('\n class"slds-button--brand" \n', 'uri', connection);
+    if (args) {


@engai I think you should also assert that args is not null in the tests.
Because if it turns out that args is null, then if will skip the if(args) {...} and then the test will pass, right?

Right. Pushed a fix to assert when null

ruthemmanuelle · 2017-09-13T00:48:24Z

packages/salesforcedx-vscode-lightning/README.md

-   * Code actions available on click
-   * Linter won't run if SLDS is included as a static resource in your project
+   * Code actions are available on click
+   Note: The linter won't run if SLDS is included as a static resource in your project.

   ![SLDS Linter detecting deprecated '--' class name syntax](https://raw.githubusercontent.com/forcedotcom/salesforcedx-vscode/ayesha/add-slds-linter/packages/salesforcedx-vscode-lightning/images/lightning_slds.png)


We should take a new screenshot that includes the updated messages. We'll also need to update this path to use /develop/ instead of /ayesha/add-slds-linter/, but if we do that before merging in this PR the image won't display. It would be good if the resolution and text size of the screenshot approximately matched the other screenshots in this README.

ruthemmanuelle · 2017-09-13T00:51:11Z

packages/salesforcedx-slds-linter/src/messages/i18n.ts

  fix_all:
  'Fix all auto-fixable problems',
  fix_error:
-  'Failed to apply SLDS Validator fixes to the document',
+  'Failed to apply SLDS Validator fixes to the document.',


Do we have any idea what would have caused this problem? Can we suggest a solution? Also, I added a period, but if this error displays in the same format as the other messages in this section we can remove the period. VS Code seems to use sentence-case capitalization and no end punctuation for these contextual messages.

It will only fail if the edit function doesn't successfully run. This wouldn't be a problem that the end user can fix. One thing we can suggest is filing an issue on Github with us.

It doesn't display along with the other messages, so the period should be okay

vazexqi · 2017-09-13T16:49:48Z

packages/salesforcedx-slds-linter/src/server/index.ts

+            break;
+          }
+          default: {
+            codeMessage = 'same problems';


Is there a reason this one is not done using nls.localize?

vazexqi · 2017-09-13T16:52:28Z

packages/salesforcedx-vscode-lightning/src/index.ts

+  vscode.workspace.findFiles('**/staticresources/*.resource').then(
+    // all good
+    (result: vscode.Uri[]) => {
+      for (let i = 0; i < result.length; i++) {


Cosmetic: Is there a reason we use for(let i = 0...) instead of the for(const a of blah)?

vazexqi · 2017-09-13T17:28:41Z

packages/salesforcedx-slds-linter/src/client/index.ts

+  // Options to control the language client
+  const clientOptions: LanguageClientOptions = {
+    // Register the server for plain text documents
+    documentSelector: ['plaintext', 'html'],


@ayeshakmaz - I think we only want HTML, right? No need for it to scan plaintext?

Yeah, we can take out the plain text option

You need to have it in an actual vscode extension to be able to access the vscode namespace.

@W-4267770@

vazexqi · 2017-09-13T23:27:19Z

Merging this in! Thanks @engai and @ayeshakmaz

vazexqi mentioned this pull request Sep 7, 2017

Ayesha/add slds linter #78

Closed

5 tasks

vazexqi reviewed Sep 7, 2017

View reviewed changes

vazexqi reviewed Sep 11, 2017

View reviewed changes

vazexqi suggested changes Sep 12, 2017

View reviewed changes

vazexqi reviewed Sep 12, 2017

View reviewed changes

vazexqi reviewed Sep 13, 2017

View reviewed changes

ruthemmanuelle reviewed Sep 13, 2017

View reviewed changes

vazexqi reviewed Sep 13, 2017

View reviewed changes

vazexqi approved these changes Sep 13, 2017

View reviewed changes

vazexqi reviewed Sep 13, 2017

View reviewed changes

ruthemmanuelle approved these changes Sep 13, 2017

View reviewed changes

Ayesha Mazumdar and others added 19 commits September 13, 2017 16:24

Add SLDS Linter

52e20b7

Normalizing paths

6eeb5a1

Fixing test workspace

6dff443

Joining paths

4c0705b

Move deprecatedClassName.test.ts to the proper extension

e5a3e5d

You need to have it in an actual vscode extension to be able to access the vscode namespace.

Clean up package.json

c25bda6

Forget apex.db

c6b1748

Fix lint error

7f0d079

Changed command name

5b3b4f7

Fixed sfdx deprecated class command test

24636c7

Add DemoComponent.cmp to system-tests

913588f

Closing editor after tests

da5acfd

Adding linter tests

8c4014d

Update messages and README

4ef2ee1

@W-4267770@

Adding assert for case of null arguments in connection

0a7adee

Adding new README image for lightning slds linter

773b611

Making cosmetic changes

2c14bc6

Remove plaintext option from document selector

08d0332

Make the vscode:package work on Node 7

59f7bd4

vazexqi force-pushed the WIP branch from be9ee8e to 59f7bd4 Compare September 13, 2017 23:26

vazexqi merged commit ceca1ad into develop Sep 13, 2017

vazexqi deleted the WIP branch September 13, 2017 23:27

vazexqi mentioned this pull request Sep 15, 2017

Create SLDS Linter to check for deprecated class names #25

Closed

randi274 mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade @salesforce/eslint-config-lwc from 3.3.3 to 3.5.1 #4965

Merged

Add SLDS Linter #101

Add SLDS Linter #101

Conversation

engai commented Sep 7, 2017

What does this PR do?

What issues does this PR fix or reference?

Choose a reason for hiding this comment

vazexqi commented Sep 7, 2017

engai commented Sep 7, 2017

vazexqi commented Sep 7, 2017

codecov bot commented Sep 8, 2017 • edited Loading

Codecov Report

codecov bot commented Sep 8, 2017

Codecov Report

engai commented Sep 11, 2017

vazexqi commented Sep 11, 2017

engai commented Sep 11, 2017

Choose a reason for hiding this comment

engai commented Sep 11, 2017

vazexqi commented Sep 11, 2017

engai commented Sep 11, 2017

vazexqi commented Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

vazexqi commented Sep 11, 2017

engai commented Sep 12, 2017

vazexqi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engai commented Sep 12, 2017

vazexqi commented Sep 12, 2017

ayeshakmaz commented Sep 12, 2017

vazexqi commented Sep 12, 2017

vazexqi commented Sep 12, 2017

Choose a reason for hiding this comment

vazexqi commented Sep 12, 2017

engai commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruthemmanuelle Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vazexqi commented Sep 13, 2017

codecov bot commented Sep 8, 2017 •

edited

Loading

vazexqi commented Sep 11, 2017 •

edited

Loading

ruthemmanuelle Sep 13, 2017 •

edited

Loading