search

Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.1k stars 1.21k forks source link

[Doc][README] trouble shooting section should use `require` if the examples in README are JavaScript #15746

Closed jeremymeng closed 8 months ago

jeremymeng commented 3 years ago

We have seen in UX Study that participants copied and pasted the code into a NodeJS javascript file but the code didn't work because it's using import which doesn't work in default NodeJS project. We should update those to use const { setLogLevel } = require("@azure/logger");

ramya-rao-a commented 3 years ago

@deyaaeldeen Do you think we can have a linter rule for our package readmes to catch such cases?

deyaaeldeen commented 3 years ago

@ramya-rao-a if you mean just checking for import syntax, I think we can. But if we want to make sure the code snippet as a whole runs fine, this is another story.

ramya-rao-a commented 3 years ago

Yes agreed. I meant to only catch the import syntax

ramya-rao-a commented 3 years ago

Code pointers for anyone wanting to fix this:

jeremymeng commented 3 years ago

eslint might not be able to work on markdown files?

deyaaeldeen commented 3 years ago

@jeremymeng I think something like this could work: https://github.com/eslint/eslint-plugin-markdown

WeiJun428 commented 2 years ago

I would like to work on this as suggested by @deyaaeldeen though I think I need some clarification. I am aware that I should change all import to require but does this only apply to logger as mentioned above? And can I get some examples in the repo for investigation? Thank you!

deyaaeldeen commented 2 years ago

@WeiJun428 the idea is to make sure the trouble shooting section in every package's readme file uses CommonJS module syntax so that Node.JS users could run the code without issues. For example, see Text Analytics's troubleshooting section, it uses ES modules, but we would like that to be rewritten to use CommonJS syntax (require) instead.

deyaaeldeen commented 2 years ago

@jeremymeng I think it makes sense to be consistent about the module syntax across the whole readme file, do you think there're any downsides for using CommonJS throughout? /cc @xirzec @witemple-msft

jeremymeng commented 2 years ago

do you think there're any downsides for using CommonJS throughout

I prefer CommonJS too. Ideally customers should be able to copy and paste code snippets without running into problems.

xirzec commented 2 years ago

I agree that we should switch to CommonJS for code snippets (at least until NodeJS more wholeheartedly embraces import syntax)

deyaaeldeen commented 2 years ago

@jeremymeng @xirzec thanks!

@WeiJun428 ok let's generalize this issue to be about ensuring using require syntax in all code snippets for any imported package.

WeiJun428 commented 2 years ago

Hi, I would like to get some clarification on where to incorporate eslint-plugin-markdown plugin into the repo. Specifically, I am wondering whether should I put my config in .eslintrc.json or azure-sdk-base.ts in eslint-plugin-azure-sdk, or both? (I noticed they works on very different environment but what are their main differences?)

deyaaeldeen commented 2 years ago

@WeiJun428 thanks for following up! azure-sdk-base.ts is the place for configuring 3rd party plugins so I believe this is what you're looking for. On the other hand, .eslintrc.json is used to lint the source code for the plugin itself only.

WeiJun428 commented 2 years ago

Thanks @deyaaeldeen for your answer! Now, I am trying to use no-restricted-import to ban any import statement in md file. But I noticed that I should probably modify the existing scripts in each package.json, specifically:

    "lint:fix": "eslint package.json api-extractor.json src test --ext .ts --fix --fix-type [problem,suggestion]",
    "lint": "eslint package.json api-extractor.json src test --ext .ts",
    // Probably need to add *.md to target files and --ext .md to lint md files at root of each sdk

so that rush lint and rushx lint will work as expected. Am I understanding it correctly? If so, how should I do that?

deyaaeldeen commented 2 years ago

@WeiJun428 You're right that current npm linting scripts ignore the README.md files so they will have to be updated. Instead of adding all .md files, let's add README.md only to the linting commands:

 "lint:fix": "eslint package.json README.md api-extractor.json src test --ext .ts --fix --fix-type [problem,suggestion]",
 "lint": "eslint package.json README.md api-extractor.json src test --ext .ts",
jeremymeng commented 2 years ago

@WeiJun428 I created a feature branch feature/enable-linting-readme. You can create PR targeting that branch for the rest of packages so we can avoid triggering massive CI validations.

WeiJun428 commented 2 years ago

@jeremymeng I didn't see the feature branch feature/enable-linting-readme on my end. I wonder where am I supposed to find it?

jeremymeng commented 2 years ago

@WeiJun428 This could be a permission issue. I will try to find a solution, meanwhile you can work on your own branch.

jeremymeng commented 2 years ago

@WeiJun428 It should be available now at https://github.com/Azure/azure-sdk-for-js/tree/feature/enable-linting-readme

WeiJun428 commented 2 years ago

@jeremymeng I wonder if there is a good way to track the progress of enable linting readme in all sdk? Something like a list or checkbox of all to-be-fixed sdk? Should we enable it in tools such as eslint-plugin-azure-sdk and dev-tool as well?

jeremymeng commented 2 years ago

@WeiJun428 good question! We don't have inventory of non-generated SDK libraries. Let me double check to see if I can find anything helpful. The main goal of this work is helping SDK customers who copy and paste code. It doesn't really apply to internal tools so we don't need to enable this rule for them.

jeremymeng commented 2 years ago

@weiThe list below should be pretty close

WeiJun428 commented 2 years ago

Thanks! @jeremymeng. I have tried to create a PR targeting that branch but I noticed that it also includes 12 other commits perhaps due to difference between the versions of repo that I fetch and the feature branch itself. Would that be an issue?

jeremymeng commented 2 years ago

@WeiJun428 Let me see if I can update the feature branch to also include those changes.

topshot99 commented 1 year ago

@jeremymeng is this issue resolved? If so, can we close it?

github-actions[bot] commented 8 months ago

Hi @jeremymeng, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.