Skip to content

[heft] Add typings options#2305

Merged
halfnibble merged 6 commits into
microsoft:masterfrom
halfnibble:halfnibble/add-typings-options
Oct 23, 2020
Merged

[heft] Add typings options#2305
halfnibble merged 6 commits into
microsoft:masterfrom
halfnibble:halfnibble/add-typings-options

Conversation

@halfnibble

Copy link
Copy Markdown
Contributor

This PR makes 2 changes.

  1. Supports undefined as a value returned by parseFileAndGenerateTypingsAsync. When undefined is returned, no typings file is generated.
  2. Adds the "fileExtensions" configuration option to the SassTypingsGenerator. This is so repos that only want to process Sass Modules may do so (e.g. by configuring extensions as ["module.sass", "module.scss"]

Comment thread apps/heft/src/plugins/SassTypingsPlugin/SassTypingsGenerator.ts
);
const generatedTsFilePath: string = this._getTypingsFilePath(locFilePath);

if (typingsData === undefined) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment what cases this would return undefined and expected behaviour


/**
* Files with these extensions will pass through the Sass transpiler for typings generation.
* Defaults to ["sass", "scss", "css"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For consistency with this, let's require that these start with a .

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you also make sure that works with typings-generator. Maybe we even require that in typings-generator?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The TypingsGenerator class appears to have logic to test for the . and add it if it's missing.
Everything seems to work fine after adding the .

Comment thread apps/heft/src/plugins/SassTypingsPlugin/SassTypingsGenerator.ts
Comment thread apps/heft/src/schemas/sass.schema.json Outdated
"description": "Files with these extensions will pass through the Sass transpiler for typings generation.",
"items": {
"type": "string",
"pattern": "[^\\\\]"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's update this pattern to something like

Suggested change
"pattern": "[^\\\\]"
"pattern": "^\\.[A-z0-9-._]*[A-z0-9-]+$"

This says:

  • Must start with a .
  • Other characters must be alphanumeric, -, _, or . (except for the last character)

That doesn't cover 100% of legal file extensions, but it should be pretty close.

It'd also be good to update this expression. Might be a good idea to extract this to a resource in a shared schema file.

Comment thread apps/heft/src/templates/sass.json Outdated
Comment on lines +30 to +32
// "sass",
// "scss",
// "css

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// "sass",
// "scss",
// "css
// ".sass",
// ".scss",
// ".css

@iclanton iclanton changed the title Add typings options [heft] Add typings options Oct 23, 2020
Comment thread apps/heft/src/schemas/sass.schema.json Outdated
"description": "Files with these extensions will pass through the Sass transpiler for typings generation.",
"items": {
"type": "string",
"pattern": "^\\.[A-z0-9-._]*[A-z0-9-]+$"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"pattern": "^\\.[A-z0-9-._]*[A-z0-9-]+$"
"pattern": "^\\.[A-z0-9-_.]*[A-z0-9-_]+$"

"items": {
"type": "string",
"pattern": "\\..+"
"pattern": "^\\.[A-z0-9-._]*[A-z0-9-]+$"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"pattern": "^\\.[A-z0-9-._]*[A-z0-9-]+$"
"pattern": "^\\.[A-z0-9-_.]*[A-z0-9-_]+$"

@halfnibble halfnibble merged commit bfca6e3 into microsoft:master Oct 23, 2020
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.

3 participants