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

[ESLint Plugin] Enforce inclusion of dist-esm and exclusion of src in files field in package.json #11411

Merged

Conversation

deyaaeldeen
Copy link
Member

@deyaaeldeen deyaaeldeen commented Sep 23, 2020

This change updates the ts-package-json-files-required rule to check for the absence of src and dist-esm in the files list in package.json since sourcemaps now have the sources embedded. It is based on discussion with @xirzec here #11273 (comment). Related issue: #7615.

TODO in this PR: update every package's package.json to conform to the final rule before merging.
After discussion with @willmtemple, @bterlson, and @xirzec here, this PR updates the eslint plugin to make sure dist-esm is included and src is not in package'json's files list.

@ramya-rao-a
Copy link
Contributor

+1 on fixing all the package.json files in the same PR

Copy link
Contributor

@willmtemple willmtemple left a comment

Choose a reason for hiding this comment

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

I don't think we can remove dist-esm. External bundlers rely on having our ESM sources to build us for different targets like browsers, and our browser mappings rely on having the ESMs in the package.

@bterlson
Copy link
Member

bterlson commented Sep 23, 2020

We should not remove dist-esm. It is not just for source mapping. It is preferred by bundlers. Some of the rationale is documented here, feel free to send a PR either updating this or removing the guideline with rationale!

@xirzec
Copy link
Member

xirzec commented Sep 23, 2020

Let's change this to remove src but leave dist-esm\src

@deyaaeldeen deyaaeldeen changed the title [ESLint Plugin] Enforce exclusion of dist-esm and src in files field in package.json [ESLint Plugin] Enforce inclusion of dist-esm and exclusion of src in files field in package.json Sep 24, 2020
@deyaaeldeen
Copy link
Member Author

deyaaeldeen commented Sep 24, 2020

Thanks @willmtemple, @bterlson, and @xirzec for the feedback! Please take another look.

Hi @XiaoningLiu, would you please confirm if source files need to be shipped in storage-datalake package? I am removing them here: 7af9e2e.

Copy link
Contributor

@willmtemple willmtemple left a comment

Choose a reason for hiding this comment

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

Left some comments with some nitpicks about code style and a couple of places where I think this could benefit from extra documentation, overall happy with the change to the linter's behavior.

Copy link
Contributor

@willmtemple willmtemple left a comment

Choose a reason for hiding this comment

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

Overall happy with this rule change, but I'd like to suggest making sure that this still works with packages that use shared code, such as keyvault (via keyvault-common) and storage. I think the output structure of their dist-esm is different due to the deeper-rooted source tree.

I think we should require all of dist-esm rather than just dist-esm/src.

* files list by the fixer.
* @param pat - A pattern that is missing from the files list
*/
function buildFixRequiredPattern(pat: string): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

This really helped clarify the purpose of the map above, thanks!

Copy link
Contributor

@willmtemple willmtemple left a comment

Choose a reason for hiding this comment

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

Just spoke with @deyaaeldeen and he clarified that the current behavior is not to requite dist-esm/src, but to use it as the default value, but anything that begins with dist-esm will be accepted. I'm good with that.

@deyaaeldeen deyaaeldeen merged commit 39cf3a2 into Azure:master Oct 14, 2020
@deyaaeldeen deyaaeldeen deleted the eslint-plugin-drop-files-dist-esm branch October 14, 2020 15:57
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.

5 participants