[Package Importer] Embedded Host #260
Conversation
|
Review comments were addressed, but I'll wait to request re-review until after the entry point logic is ironed out. |
* main: add comment update actions Specify node versions a la sass/sass-spec#1957 Work around recent breaking change in Node update package.json Test against Node20, remove Node14. Add semicolons and remove duplicate TS config
jgerigmeyer
force-pushed
the
feature.package-importer
branch
from
ceda6f8
to
2417f0b
Compare
…o feature.package-importer
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…o feature.package-importer
lib/src/importer-registry.ts
Outdated
| const entryPointDirectoryKey = Symbol(); | ||
|
|
||
| export class NodePackageImporter { | ||
| [entryPointDirectoryKey]?: string; |
There was a problem hiding this comment.
I believe this can be readonly and non-nullable, since it's always set in the constructor (if it doesn't throw an error).
There was a problem hiding this comment.
Adjusted in 1f4489c. Because the Sass NodePackageImporter type does not include this property, TypeScript complains when calling register() because this NodePackageImporter no longer matches the expected type from Sass. I addressed that here by adding importer as Importer<sync> | FileImporter<sync> | NodePackageImporter, but if this trade-off isn't worth it I can revert this commit and go back to an optional entryPointDirectoryKey.
There was a problem hiding this comment.
I'd definitely prefer to have this readonly and non-nullable, since that's a direct representation of the actual state of the class. The static errors raise an interesting issue, though—because TypeScript checks types structurally, and the existing NodePackageImporter type from the Sass package has no properties, it will effectively allow any object to be passed as an importer. It should probably have its own secret symbol property to avoid this, which will in turn mean that we will need a cast here no matter what.
There was a problem hiding this comment.
@nex3 Ah, ok -- that makes sense. So adding a private readonly non-nullable Symbol key to the definition in the Sass repo, correct?
There was a problem hiding this comment.
@nex3 I addressed this in the Sass repo: https://github.com/sass/sass/pull/3728/files/b9ed7455df502b8bd3e902c793d5d8666bc2d468..b09caefbce9b1d2c18da8640b4140dc52c5765cc
sass/sass#2739
Blocked until proposal is accepted.