refactor: Be stricter about imports #485
Conversation
We don't have to import `getTagID` anymore after this change.
Most of them are marked as `@internal`, which will hide them from the documentation
We always aliased `NAMESPACES` as `NS`, so let's make that the official name.
| @@ -1,4 +1,5 @@ | |||
| export enum NAMESPACES { | |||
| /** All valid namespaces in HTML. */ | |||
| export enum NS { | |||
There was a problem hiding this comment.
We always aliased NAMESPACES as NS, even in this file. This makes it the actual name.
| @@ -18,7 +18,9 @@ | |||
| ], | |||
| "license": "MIT", | |||
| "main": "dist/index.js", | |||
| "exports": "dist/index.js", | |||
| "module": "dist/index.js", | |||
| "types": "dist/index.d.ts", | |||
There was a problem hiding this comment.
by setting this, does that mean importing an exact file will fail in typescript? since it'd look for the definition in here rather than {whatever-you-imported}.d.ts. maybe thats your intention? still trying to understand the overall change tbh so could've just misunderstood
There was a problem hiding this comment.
With this PR, exact file imports are no longer allowed — all you can do is import the modules. This is done through the exports field.
As for the types field — from my understanding, this is for TS to figure out how to resolve a plain module import (eg. import ... from 'parse5'). TS should use co-located .d.ts files when you actually import a file.
There was a problem hiding this comment.
because we don't bundle anything, i feel like we don't need it. typescript will already detect that there's an index.d.ts for the index.js. using a bare import will resolve main or whatever else, and then typescript will find the parallel d.ts file iirc.
There was a problem hiding this comment.
I am honestly not sure if we need it or not. Eg. tsc might be able to pick up the type definitions, but will NPM show the TS badge if there is no types field in the package.json?
My gut instinct is to keep this field, just to be safe.
There was a problem hiding this comment.
i just tried it on a package, with types set using a direct import of a module. it seems to work fine, so i think you're ok to leave this here & yeah npm checks explicitly for types/typings AFAIK
Addresses #453 (comment). Changes are in three categories:
parse5package (many of them marked as@internal, which hides them in the generated documentation). This way, we won't need deep exports. I'm not entirely happy with the amount of API surface this exposes, but marking these as@internalshould allow us to introduce some potentially breaking changes in non-major versions.hasUnescapedText, which made sense as part of the HTML file.import * as parse5in most places, and we are explicit with type imports.Additionally, all
package.jsonfiles have been updated.Fixes #301