- improve grammar in several places
- mostly add "the" and commas in several places where they were missing
- and put a period or semicolon in a few places where there was a comma splice
- for "deep merge", parens were used, whereas a colon is a better fit
- consistently use "(see #x)", instead of sometimes using parens and other times using commas
- also change "only if `useTsconfigDeclarationDir`" to "unless `useTsconfigDeclarationDir`" for clarity
- have looked at this many times and thought something was unintuitive about this: `useTsconfigDeclarationDir` defaults to `false` after all
- "typescript" -> "TypeScript" as a proper noun
- "js" -> "JS" similarly
- "es6" -> "ES6"
- "rollup" -> "Rollup"
- "rollup watch" -> "Rollup's watch mode"
- "work directory" -> "working directory" (that's what the "w" stands for)
- improve formatting in several places
- split up paragraphs a bit more neatly as a single new line in Markdown is just rendered as a space
- similar to my previous commit for `emitDeclarationOnly`
- and apparently I missed two sentences in the "Declarations" section there too
- for `tsconfig`, `objectHashIgnoreUnknownHack`, and `typescript`, actually add a double new line
- to break up the paragraph for better rendered readability
- don't skip heading level for "Some compiler options" -- this should be an h3, not an h4
- remove inconsistent period in one of the headings as well (and headings aren't normally supposed to have periods)
- consistently use backticks when referencing plugin, `tsconfig` options, etc
- e.g. `tsconfig`, `include`, `exclude`, `node_modules`, etc
- also, for `allowJs`, put the proper `**/node_modules/**/*` `exclude` as an example similar to the `**/*.js+(|x)`
- as plain `node_modules` won't work as I've recently found out
- consistently use 1 tab indent for plugin options' descriptions, instead of a few 2 tab indents
- for "Requirements" section, use bullets instead having everything on one line
- the way it was written seemed like they were intended to be on different lines, but a single new line in Markdown is just rendered as a space (per above)
- bullets are a little better than just new lines as well
- add TSConfig Reference links to several `tsconfig` options mentions
- `extends`, `declarationDir`, `declaration`, `declarationMap`, and `emitDeclarationOnly`
- also added the mention of `extends` in "chaining tsconfigs"
* fix(diagnostics): workaround Rollup duplicating error messages
- per my investigation in the linked issues, it seems like Rollup has a bug where it duplicates some error message
- this occurs when the error has a stack (or frame) which contains the error message itself
- Rollup prints _both_ the error message _and_ the stack in that case, causing duplication
- this fix adds a workaround for this upstream Rollup bug
- it detects if there is a stack and if the message is duplicated in the stack
- if so, it removes the duplication in the stack
- this workaround should be forward-compatible if this behavior is fixed upstream
- this code should just end up re-throwing in that case (effectively a no-op)
* fix watch mode by spreading err
- apparently Rollup attaches several properties to the error object, including `watchFiles`
- so removing them / not spreading causes watch to just stop working
here are some of the additional properties I logged out, for example:
```js
{
id: '/project-dir/src/difference.ts',
hook: 'transform',
code: 'PLUGIN_ERROR',
plugin: 'rpt2',
watchFiles: [
'/project-dir/src/index.ts',
'/project-dir/tsconfig.json',
'/project-dir/src/types.ts',
'/project-dir/src/difference.ts'
]
}
}
```
- per https://www.typescriptlang.org/tsconfig#pretty
- TS 2.9 changed the default to `true`: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-9.html#--pretty-output-by-default
- so this is a legacy remnant of older versions of TS
- this is why `tsc` by default pretty prints, even when `pretty` is unset
- only if it is _explicitly_ set to `false` does it not do that
- so change rpt2's diagnostic printing to be `pretty` by default as well
- I think this is a pretty _big_ DX improvement, to be honest
- I always thought something was up with the error reporting, but saw that there was code for `pretty`, so didn't quite connect the dots until now
- that while there was code for it, the default was wrong, and so unless I set `pretty: true`, I wasn't getting easier to read printing
- and given that `pretty: true` is the default, that's a redundant / unnecessary option to set in `tsconfig` that I normally wouldn't ever re-set
* fix(cache): invalidate declarations when imports change
- previously, the `checkImports` flag was set to `true` for type-checking, but `false` for compilation
- I _believe_ it is `false` because the compiled JS shouldn't change if an import changes
- though I'm not sure if that was the original intent behind the code
- problematically though, compilation results can include declarations, and those _can_ change if imports change
- for instance, the types of an import can change the declaration that is output
- so now, only set it to `false` for compilation if declarations are _not_ needed
* add `!isolatedModules` check as well
- ezolenko gave a good example of enums, which can indeed cause the compiled JS code to change based on imports
- so also check `!isolatedModules` as well
- I thought it might be the case that the code wouldn't handle other edge cases, but couldn't think of one off the top of my head
- ironically, I compared it to Babel, which transpiles per file, and Babel _requires_ `isolatedModules` to work
- it is set to `true` in every usage, so just always check the new cache and remove the parameter entirely
- rewrite the comments to reflect this change
- and condense them a bit
- modify tests a bit to reflect this change as well
- keeping it short and simple for now, but this is basically the format that the majority of my PRs follow
- can add more sections or optional sections as needed in the future
- this should help keep PRs consistent, which makes them easier to read and review
- (similar rationale as that for code style consistency)
- should also help preserve the historical context and rationale behind PRs
- which also makes reviews _significantly_ easier and involve less guesswork
- which can be very important for figuring out the intent of code
- which can be particularly important when trying to decipher if a piece of code was written intentionally for a specific purpose, or accidentally has a bug in it, or is redundant, etc
- while comments serve a similar function, not all code is commented and the overall purpose of a PR may not necessarily fit into any line of the code
- or it may only be relevant historically, and not necessarily relevant to a piece of code's current functionality
- etc etc etc etc
- also requests a small bar of quality from PR authors as well
- if there is no cache, we don't need to do any operations on a cache at all, so we can just totally skip all the cache operations and return early
- this should be a perf improvement, at the very least on memory usage, as lots of stuff isn't created or processed now
- this may make `clean: true` a more optimal choice for smaller projects, as the FS usage when writing to cache may be slower than the small amount of compilation and type-checking required in small projects
- to start, from the constructor, the only necessary piece when `noCache` is the dependency tree
- this is needed for `walkTree` and `setDependency` to function, but otherwise is unused when `noCache`
- this _might_ be able to be further optimized to remove the graph entirely when `noCache`, but that is saved for potential future work and not part of this commit
- so, we can just move the tree creation further up in the constructor as its previous ordering within the constructor does not actually matter
- once this is done, we can just early return when `noCache` instead of doing all the cache-related actions, since they're not neeeded when there is no cache
- no need to set `cacheDir` or any hashes etc since they're not used
- note that `clean` only uses `cachePrefix` and `cacheRoot`, which are already set, and does not use `cacheDir`
- no need to `init` the cache as it's not used
- also slightly change the ordering to move `init` right after its prereqs are done, i.e. setting `cacheDir`, `hashOptions`, etc
- just keeps with the flow instead of calling it in the middle of the ambient type processing
- no need to check ambient types as that is only used for cache invalidation (marking things dirty), which is not used when there is no cache
- note that `isDirty` is literally never called when `noCache`
- from there, since we don't call `checkAmbientTypes` or `init` when `noCache` (the constructor is the only place they are called and they are both `private`), we can entirely remove their `noCache` branches
- fairly simple for `checkAmbientTypes`, we just remove the tiny if block that sets `ambientTypesDirty`, as, well, "dirty" isn't used when there is no cache
- for `init`, this means we can entirely remove the creation of `NoCache`, which isn't needed when there is no cache
- that means we can also remove the implementation and tests for `NoCache`
- and the reference to it in `CONTRIBUTING.md`
- in `done`, we can also simply skip rolling caches and early return when there is no cache
- the only other tiny change is the non-null assertions for `ambientTypes` and `cacheDir`
- this matches the existing, simplifying non-null assertions for all the caches, so I did not workaround that
- _could_ set a default of an empty array for `ambientTypes` etc to workaround this, but thought it better to match existing code style and not add new things
- this also matches the behavior, as while `ambientTypes` and `cacheDir` could be `null`, this is only if there is no cache, in which case, they are never used
- basically, refactor the `getDiagnostics` a tiny bit to also handle what the `getCompiled` function needs
- then just call `getCached` from both instead
- `getDiagnostics` and `getCompiled` were near identical except for the cache they used and `checkImports`, which are both parameters now
- `getCompiled` also had a logging statement (called in both `noCache` and w/ cache branches), which was moved to before the call to `getCompiled` instead
- the `type` param was composed into another function in `getDiagnostics` before the call too `getCached` now
- this simplifies all the cache code to one main function now
- remove the `markAsDirty` call under `noCache`, as well, "dirty" has no meaning for `noCache` anyway
- this simplifies that one `if` statement now too
- the `fileNames` Set that was previously introduced in c86e07bc caused a regression during watch mode
- this is because `setSnapshot` was updated to call `this.fileNames.add`, but `getScriptSnapshot` was not
- instead of updating both to be the same, we can just call `setSnapshot` from `getScriptSnapshot` as this code is supposed to be identical
- also rename `data` -> `source` for consistency and clarity (`source` is a more specific name)
* docs/clean: formally deprecate `rollupCommonJSResolveHack`
- this has had no effect since 6fb0e75f5328666dca8ff7b11bc956096351a3eb, released in 0.30.0
- that changed the code to always return OS native paths via the NodeJS Path API
- so setting `rollupCommonJSResolveHack` would make no difference either `true` or `false`
- effectively, it's as if it's always `true` now
- formally state now that this is deprecated in the docs
- as well as when that occurred and what it means
- also add a warning in `options` similar to the existing one for `objectHashIgnoreUnknownHack`
- remove the `resolve` dependency as well
- it turns out something in the devDeps still uses it, so it didn't get fully removed in the `package-lock.json`
- `resolve` was never needed anyway as we could've used NodeJS's native `path.resolve` or `require.resolve` instead
- `resolve` was created for `browserify` after all, where one can't use NodeJS APIs
- but we run on NodeJS and can and already do use NodeJS APIs, including both `path.resolve` _and_ `require.resolve`
- I actually started this commit just to remove the dep, then realized the entire code path is obsolete
* fix lint error -- resolved can now be const
- previously the `allImportedFiles` Set was _basically_ used to skip any files that were imported by other plugins
- it only filtered out on `importer` (not `importee`)
- this is a bit of a problem though, as other plugins that create JS code, or `allowJs` JS code, or heck, even regular JS code that Rollup processes without plugins, may actually import TS files
- and Rollup's plugin system is designed to handle that scenario, so rpt2 actually _should_ resolve those files
- notably, rpt2 already _transforms_ those files, it just didn't resolve them
- which is why using `@rollup/plugin-node-resolve` with a `.ts` extension solved this as a workaround
- the file would be resolved by `node-resolve` then actually transformed by rpt2
- but rpt2 should resolve TS files itself, `node-resolve` shouldn't be necessary
- this caused the issue that extensionless imports from such files wouldn't get resolved to TS files and may cause Rollup to error
- in particular, this popped up with extensionless imports of TS files from Svelte files
- `resolveId` is actually the last remaining place where `allImportedFiles` was used
- so after removing it from here, we can just remove it entirely
- which should be a small optimization
- for reference:
- `allImportedFiles` would be any `include`d files as well as TS files that have been transformed (at this point in the build) and their references
- this is a pretty gigantic list as is, so I'm actually not sure that removing this is actually that big of a de-opt
- and it only affects mixed TS / non-TS codebases too
- this seems to have been added in b0a0ecb5ee8752a1e60962036beb52e9f5dcfff9, but that commit doesn't actually seem to fix the issue it references
- see my root cause analyses in the issues
- when `emitDeclarationOnly` is set, only perform type-checking and emit declarations, don't transform TS
- `result.code` actually doesn't exist when `emitDeclarationOnly` is set anyway
- this caused a confusing situation for users who were trying to use Babel (with Babel plugins) on TS and only use rpt2 for declarations
- instead of getting any JS code, they would just get empty chunks, bc `result.code` is `undefined`
- that's kind of buggy, it should probably either be forced to `false` or do what we're doing now, which is likely more intuitive / intended
- so now, instead of getting an empty chunk, rpt2 will just pass to the next plugin, allowing for other plugins on the chain to process TS
- this opens up some new use cases, like using in tandem with Babel plugins (as the issue illustrates) or using for type-checking and declaration generation while using Vite / ESBuild for compilation
- add a new paragraph to "Declarations" docs about this new feature and what it does and some examples of how it could be used
- note that these are unexplored / untested integrations, so point that out in the docs too
- also modify a self-reference further up in the docs to use code backticks for `rollup-plugin-typescript2`
- while at it, also add a line about declaration maps
- and reformat the existing paragraph a bit to match the style and improve readability
- one sentence per line, which is all on the same paragraph in Markdown anyway
- add a `<br />` element to add a new-line _within_ the paragraph for better readability / spacing
- Markdown supports this only with two trailing spaces, which is difficult to see and the trailing whitespace can be trimmed by editors, so prefer `<br />`
* refactor: split out a common typecheckFile func
- this is used in 3 places and going to be more for the code I'm adding
to fix type-only imports
- so DRY it up and standardize the functionality too
- some places had `noErrors = false` in one place while others had
it in another
- same for `printDiagnostics`
- but the ordering actually doesn't matter, so just keep it
consistent and the same
- and then can split a common function that does both out
- technically, now getDiagnostics is _only_ used in typecheckFile, so
I could link to the two together, but I'm refactoring that one up
a little
- but this a good, small example of how refactoring one part of a
codebase can make it easier to identify more similar pieces and then
refactor even more
* fix lint error on shadowed name
* refactor(cache): simplify `clean` method
- `cache().clean()` is only called during instantiation in `options`, so we can instead just call it inside of the constructor
- there is no need to call `this.init()` in `clean` as it's only used in the constructor anyway, which already calls `this.init()`
- and if `noCache` is `true`, `init` is basically a no-op too
- so technically we don't need to call `init` _at all_ if `noCache`, but that's a larger refactor that I'm splitting into a separate commit/PR
- now that `clean` is just one conditional, we can invert it and return early instead
- it's also not necessary and less efficient to call `emptyDir` before `remove`; `remove` will unlink all the contents anyway
- docs here: 0220eac966/docs/remove-sync.md
- though this also just normal FS behavior
- IMO, it's also not necessary to check if it's a directory, we can `remove` it either way
- and not necessary to log out if we _don't_ clean it
- then also just simplify the logic to use a `filter` instead of a nested `if`
- which we already do in several places, so this follows existing code style
* undo dir and not clean log removal
- as requested in code review, will go with a safety-first operating assumption
- as such, with safety as modus operandus, make the logging more detailed in these scenarios, since they're not supposed to happen
- and don't check code coverage on these as they're not supposed to happen in normal usage
- as I've done in other places, simplify a condition by using optional chaining syntax (over the previously necessary syntax with `&&` etc)
- also add a new line to the first `.map` here
- for style consistency and because this is a super long line
- this wasn't possible before the lodash refactor because it was all one big single statement in the chain
* refactor(cache): makeName -> createHash for clarity
- I've actually been confused multiple times as to what `makeName` does when I read through the cache
- I re-read the code and then am like "oh it's the hash"...
- so thought renaming it to `createHash` would make things **a lot** clearer
- `Name` -> `Hash`
- `make` -> `create` because that's the more common terminology in programming
- also rename variables that reference `makeName`'s return from `name` to `hash`
- for the same reason around clarity -- this way it's quicker to interpret whenever you see it too
- not to mention, `name` can be confusing since we also have `id`, which is a path that is very similar to a name too
- and lots of `fileName`s too
- so good to disambiguate/differentiate a bit
* rename object-hash default import so no shadowed variables
- use typedoc for `isDirty` comment so that it actually appears in IDEs etc
- and use `@returns` typedoc annotation for better specification
- modify the comment a little for better grammar: 3 "or"s and no commas -> 1 "or" and commas
- use more accurate terminology: "global types" -> "ambient types"
- that's also more consistent in this codebase itself, where there's a legit function named `checkAmbientTypes`
- in `init`, specify that the `RollingCache` error should never actually happen
- I was a bit confused by when this would happen, then checked the constructor, and the answer is basically never: it's an invariant / redundant error-check
- also add a new line for style consistency
- this should help make reproductions easier as it gives a quick
starting point requiring no boilerplate and is easily accessible from
a browser too
- decided to go with StackBlitz over CodeSandbox and repl.it as it's
WebContainer tech allows for running Node projects entirely _inside_
the browser (via WebAssembly)
- meaning that unlike CodeSandbox and repl.it, their platform doesn't
need to spin up and connect to a container on the backend
- so it's much more efficient, performant, and cost-effective
- and, importantly, it means you don't need an account to get started
with a Node project; you only need an account to save your work etc
- this makes a noticeable UX difference, since user funnels
generally decrease the more steps you take (especially a larger
one like creating an account)
- and we want reproductions to be as easy as possible to create
(so that people actually create them), so optimizing this flow
is important
- that being said, CodeSandbox and repl.it have much stronger OSS
presences and have open-sourced most of their core technology
- and Rollup and rollup/plugins use repl.it for reproductions:
https://replit.com/@rollup/rollup-plugin-repro
- so wanted to use those, but the UX difference was pretty
significant
- repl.it also didn't have as good a DX IMO
- have already made several reproductions of issues using this
environment as well, so can confirm that it works well!
- StackBlitz: https://stackblitz.com/edit/rpt2-repro
- GitHub: https://github.com/agilgur5/rpt2-repro
- add global version of TS as well
- with some frequency, people report not having the same issue with
`tsc`, and in some cases, that's because they're using their
globally installed version of `tsc`, which is at a different version
than their project's `tsc`
- this will help guard against that
- add OS and CPU details
- OS bc non-POSIX path separator issues with Windows are somewhat
common
- CPU in case certain architecture may have an impact, e.g. new Apple
ARM Silicon in Mac M1/M2 etc
- it really shouldn't as this is mostly a high-level library, but
some of the lower level functionality of TS and all the FS details
_could_ potentially have an impact
- add Node, Yarn, and NPM versions
- since sometimes it is an issue with the Node version or due to the
specific package manager
- e.g. Yarn workspaces, Lerna, `pnpm` symlinks etc have caused
issues before
- that being said, this command won't tell us _which_ of the package
managers the user is using, just all the installed ones' versions
- also wanted to add `pnpm` but it is not yet supported by `envinfo`
- there has been a PR out for it for a while, but hasn't been merged
by the creator :/
- thought that as it's somewhat? fresh in my head (pending if you
consider years of sporadic contributions), it would be really good to
document for new contributors who don't know where to start
- other than being good practice, thought this could be useful for a
number of reasons:
1. helpful resources like the TSConfig Reference, the TS Wiki, and the
Rollup Plugin docs, that I read through with some frequency
- (and contribute lots to the TSConfig Reference too, it's by far
the most useful resource in the ecosystem, IMO)
2. explaining directly in the docs how sparsely documented the TS
Compiler API is
- like it really makes things difficult and every time I'm looking
to contribute a bigger bugfix I look at the TS Wiki and don't
get a super helpful answer there
3. this codebase is actually fairly simple (it's the main reason I
became a contributor, as I've stated in a few issues) and truly not
that hard to get started with
- but there are some rabbit holes you can go down that can scare
away some contributors, like the cache code or the logging nuances
- so this gives a bit of a guide to not fall into some rabbit
holes and start off with the less complex bits of code
4. encourage more contributors to make some PRs like I first did!
- these steps should help in 2 ways:
1. hopefully avoid common misconfigurations from being repeatedly
opened as bug reports
- some of these have labels on them now to get a sense of how often
they pop up
2. help contributors and maintainers diagnose issues quicker and
figure out if an issue is indeed a bug
- the diagnosis questions are really common things we ask in an
issue already, so think it would work out better to get that
up-front
- we have labels for some of these too
- especially as issues can often get no response / go stale, and
if it's truly a bug, it's better to know that earlier than later
- condense the `CONTRIBUTING.md` a bit now that some of the most common
debugging steps are in the issue template
- didn't put in `npm prune` or `clean: true` _yet_, as don't want to
make the issue template _too_ big overnight
- those also aren't as common an issue and people seem to figure
out that an issue is due to caching bugs pretty often already
- probably want to move to GitHub's new beta Issue Forms moving forward,
but thought it'd be best to get the details into Markdown first,
_then_ can migrate
- and also creating Issue Forms seems to require moving to the
multiple Issue Templates format where users select a "type" of issue
(e.g. bug report vs. feature request etc), but we don't have
multiple templates yet, so that could be confusing
- no need to duplicate types this way, which can and have changed over
time -- it's always the same typings this way
- also reorganize `host.ts` to have similar categories of functions near
each other, instead of a mix of functions wherever
- similar to how I organized the tests for `host` as well
- shrink the code a bit this way too
- add a comment about `getDefaultLibFileName`'s confusing naming
pointing to the TS issues about how this is an old mistake but
changing it now would be breaking
- this is also how the TS Wiki recommends setting up hosts: https://github.com/microsoft/TypeScript/wiki/Using-the-Compiler-API#incremental-build-support-using-the-language-services
- NOTE: because of how `tsproxy` works to support alternate TS
implementations, this does require that `tsModule` _exists_ at the
time of instantiation, i.e. that `setTypescriptModule` has already
been called
- for `host.ts`, `LanguageServiceHost` is only instantiated after
`setTypescriptModule`, but for `diagnostics-format-host.ts`, it is
immediately instantiated (at the bottom of the file), hence why
`getCurrentDirectory` can't just be assigned to `tsModule.sys`
- there is a way to fix this, but the refactoring is more complex
as it would require creating in `index.ts` and then passing it
as an argument -- would want to refactor more at that point too,
so leaving that out for now in this otherwise small, isolated
refactor
- for a different, but related reason, the `host.trace` tests have to
mock `console` instead of just `console.log`, since `trace` would
just be set to the old, unmocked `console.log` otherwise
- as it's assigned directly to `console.log` now
- my previous commit was mostly focused on index.ts, this one is for
get-options-overrides -- in general, I'm just trying to make old code
more readable as I come across and then realize that the code can be
re-written better, or, in this case, a conditional inverted
- these should run through the same `filter` as runs in `transform` etc
- prior to this, the plugin `exclude` would filter files in
`transform`, meaning no JS would be output for them, but would still
output declarations for these very same files that no JS was
produced for
- (this would only happen if one were using an `include` glob or
something that made the file appear twice, i.e. once by Rollup
in `transform` and once in `parsedConfig.fileNames`)
- this change makes it so the plugin `exclude` affects both JS
and DTS output equivalently
- it was very confusing when it didn't, and users relied on setting
different `tsconfig` `exclude`s to workaround this (as a
`tsconfig` `exclude` would make the file not appear in
`parsedConfig.fileNames`)
- this checks if any of the files in `parsedConfig.fileNames` are _not_
in `allImportedFiles`, but all the files in `parsedConfig.fileNames`
are explicitly added in the `options` hook on line 98
- so this is redundant / dead code; the check will never be true
- this can be considered a remnant of an old bug as an old commit fixed
a bug with `allImportedFiles` after it was released:
f24359e668
- previously, while errors would be printed, the flag for `noErrors` was
not set to `false`, and so there would be no yellow warning about
errors
- this is mostly just fixing asymmetric UX, but personally, I actually
have missed this error before myself, so maybe this will help
alleviate that
- the snapshot is set above, so no need to get it
- also the snapshot from above is used below already, so this doesn't
seem to be intentional
- it originates from 2d330640316894752bae5ae64d94bc90652e4564 which
similarly doesn't seem intentional
- and build
- v21 broke `graphlib`'s internal usage of `lodash` with its breaking
change, and v22 has another breaking change to fix that
- notably, this was breaking rpt2 imports for some users because rpt2
bundles in `graphlib` etc, meaning the nuances of the bundling got
leaked to consumers, and, in fact, broke their builds
- another upside to unbundling
- `noEmitOnError: true` acts like `noEmit: true` when there is an error
- this is problematic because it will then cause _all_ files to have
`emitSkipped` set to `true`, which this plugin interprets as a fatal
error
- meaning it will treat the first file it finds as having a fatal
error and then abort, but possibly without outputting any
diagnostics what-so-ever as the file with the error in it may not
yet have run through the `transform` hook
- i.e. an initial file that imports an erroring file at some point
in its import chain will cause rpt2 to abort, even if that
initial file _itself_ has no type-check/diagnostic issues
- bc TS does whole-program analysis after all
- this has only been reported as an issue once so far, probably because
it defaults to `false` in TS and, as such, is rarely used:
https://www.typescriptlang.org/tsconfig#noEmitOnError
- we usually have the opposite issue, people trying to set it to
`false` (i.e. the default) because they don't realize the
`abortOnError` option exists
- add `noEmitOnError: false` to the forced options list and tests too
- add it to the docs on what tsconfig options are forced
- and add a reference to the issue like the existing options
- also reference `abortOnError` since they're commonly associated with
each other and that plugin option is often missed (per above)
- briefly explain that `noEmit` and `noEmitOnError` are `false` because
Rollup controls emit settings in the context of this plugin, instead
of `tsc` etc
- should probably watch out for when new emit settings are added to
TS, as we may want to force most with the same reasoning
- basically, general format is:
```ts
import x from "external-dep"
import y from "./internal-dep"
```
- so external deps, new line, then internal/local deps
- with some further sorting within there, like trying to keep Node
built-ins (e.g. `path`) at the top half of externals, then core deps
like `typescript`, then any other external deps
- and similar for internal deps -- core internals at the top half of
internals, then any other internal deps
- just to keep things consistent between files -- makes the top
easier to read through when it's similar between files
- also makes it easier for contributors to understand where to put
imports, as there's a sorting already there
- this is how I generally sort my imports and how I wrote most of the
unit test suite's imports as well
- there is automation for this that we should probably add once TSLint
is replaced here; some previous art:
- https://github.com/trivago/prettier-plugin-sort-imports
- https://github.com/lydell/eslint-plugin-simple-import-sort/
- Older:
- https://github.com/renke/import-sort/tree/master/packages/import-sort-style-module
- https://github.com/mcdougal/js-isort
- inspired by Python's `isort` ofc
- when using rpt2 as a configPlugin, there is no Rollup `output`, so it
will be `undefined`
- when `declarationMap: true` in `tsconfig`, this block of code is
called as a workaround due to the placeholder dir we must use for
output -- but, in this case, it errors out, since the
`declarationDir` is `undefined`
- `path.relative` needs a `string` as a arg, not `undefined`
- so skip this transformation entirely when there's no `output`, as
it doesn't need to be done in that case anyway
- this transformation code happens to have been written by me 2
years ago too, so I had fixed one bug with that but created
a different bug 😅 (fortunately one that only I have stumbled upon)
- a few `if (cond) { big block } return` could be inverted to
`if (!cond) return` then the block unindented instead
- generally speaking, this makes it a lot more readable (less
indentation, etc) and follows the existing code style in much of the
codebase, where it's a few quick one-line ifs and then just the rest
of the code
- shorten the resolvedFileName conditional by using optional chaining
- prefer the simpler `x?.y` over the older `x && x.y` syntax
- add a `resolved` variable for less repetition of the whole statement
- add a comment to the `pathNormalize` line about why it's used in that
one place and link to the longer description in the PR as well
- shorten comment about `useTsconfigDeclarationDir` so it doesn't take
up so much space or look so important as a result
- and it's easier to read this way and I made the explanation less
verbose and quicker to read too
- remove the `else` there and just add an early return instead, similar
to the inverted conditionals above
- similarly makes it less unindented, more readable etc
* - fix for vue declaration file names
* fix: use .d.ts instead of .vue.d.ts for Vue declarations
- `.vue.d.ts` was recommended by the `rollup-plugin-vue` maintainers
back when this code was originally written, but apparently it causes
problems with some imports
- plain `.d.ts` seems to work fine according to users
- and plain is the standard in TS too I believe
Co-authored-by: ezolenko <zolenkoe@gmail.com>
* fix: add `realpath` to host to properly resolve monorepos
- tested this in a pnpm repo with symlinked deps and it worked there,
so I believe this fixes all pnpm issues
- it may also fix some Lerna issues if they were due to symlinks, but
I didn't check those
- not sure about others, e.g. Rush, Yarn workspaces, Yarn PnP
- I figured out this was needed by staring at the TS source code and
then I found this line:
67673f324d/src/compiler/moduleNameResolver.ts (L1412)
- it expects `host.realpath` to be implemented for TS's `realPath` to
work correctly, otherwise it just returns the path with no
transformation (i.e. the path to the symlink instead of the
realpath)
- this is not documented _anywhere_ and we were hitting this when
calling `getEmitOutput`, before even using `moduleNameResolver`
- so I just tried implementing it... and it worked!
- notably, the other Rollup TS plugins don't implement this either???
- not sure how they don't error on this??
- note that I added a `!` as `realpath` doesn't have to be implemented
on `ts.sys`... but it is in the default implementation (see comment)
- I originally had a ternary with `fs.realpathSync` if it didn't exist
but that is literally what the default implementation uses
- can add this back in the future if it becomes an issue
* fix realpath test on windows
- the `outDir` was not normalized after the `/placeholder` part was
added to `cacheRoot`
- `cacheRoot` could have `\` directory separators on it on Windows,
which caused some tests to fail on Windows before
- tests have been normalized now too
- `expandIncludeWithDirs` used `path.join` without normalizing after
- `path.join` uses the OS's native separators (`posix.join` would do
POSIX separators only), so when the paths were already normalized
and then `path.join`ed, this would cause mixed separators on Windows
- this fixes the current CI failure on Windows in the `createFilter`
tests (`rootDirs` and `projectReferences`, which use
`expandIncludeWithDirs`)
- c.f. https://github.com/ezolenko/rollup-plugin-typescript2/runs/6516149780?check_suite_focus=true
* test: 100% coverage in get-options-overrides (createFilter)
- get the skipped test to work by using absolute paths, as that is what
the `filter` function expects (and is how it is used in this codebase)
- use the same helper func on the main `createFilter` test as well to
ensure that we're consistently testing the same paths
- (though `**/*` basically matches _everything_)
- add a test for project references as well
- and remove the `**/*` from the include for this so it doesn't match
everything
- this also tests the single string include code path as well
- add a test for the `context.debug()` statements as well
- couldn't get to 100% Funcs coverage without this
- used a simplified mock instead of actually using `RollupContext` so
that this doesn't rely on that file/unit to work
* quick fix for Windows?
- _.endsWith -> String.endsWith
- _.concat -> Array.concat
- _.each -> Array.forEach
- _.filter -> Array.filter
- _.map -> Array.map
- _.some -> Array.some
- _.has -> `key in Object`
- _.defaults -> Object.assign
- _.get -> `?.` and `??` (optional chaining and nullish coalescing)
- refactor: replace fairly complicated `expandIncludeWithDirs` func to
just use a few simple `forEach`s
- not as FP anymore, more imperative, but much simpler to read IMO
- refactor: add a `getDiagnostics` helper to DRY up some code
- also aids readability IMO
- a few places are still using lodash, but this paves the way toward
removing it or replacing it with much smaller individual deps
- _.compact still used because Array.filter heavily complicates the
type-checking currently
- _.isFunction still used because while it's a one-liner natively,
need to import a function in several places
- also the package `lodash.isFunction` is lodash v3 and quite
different from the v4 implementation, so couldn't replace with it
unfortunately
- _.merge is a deep merge, so there's no native version of this
- but we may remove deep merges entirely in the future (as tsconfig
doesn't quite perform a deep merge), or could replace this with a
smaller `lodash.merge` package or similar
- see also https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore
- rename the job a bit to match, and shrink the name a bit for
conciseness as it didn't always fit on screen in the list of checks
- this could be done in a different job as it's not necessarily required
for linting to run through the entire matrix, but this is simpler for
now as we don't have a further need for separate jobs
- and no need to re-run install etc, and the tests are fast anyway
currently, so no perf need there either
- tslib 2.4.0 is forward and backward-compatible with older and newer
Node exports mechanisms, so the Node 17 error should no longer be
present
- it has the older `./` and the newer `./*` in its package exports,
which should allow for `package.json` to be read in both older and
newer implementations
- this allows us to remove the extra dep on `@yarn-tool/resolve-package`
as well
- other than less unnecessary deps being good,
`@yarn-tool/resolve-package` is also a not well-documented package
with very few users, which does not make for a good security posture
for rpt2 (which has historically prioritized supply chain security
in other issues around deps) or, in particular, its consumers, which
there are very many of (in contrast with `@yarn-tool`)
- per my issue comment, we could also have avoided the extra dep prior
to the tslib upgrade by resolving to absolute paths, as Node only
does a "weak" encapsulation of relative imports
- test: add a small unit test for tslib.ts to ensure that this method
works and passes on different Node versions in CI
- more a smoke test that it runs at all, the testing is additional
and a bit duplicative of the source tbh
- at least as of TS 2.1: https://www.typescriptlang.org/docs/handbook/utility-types.html#partialtype
- we have a peerDep on TS >=2.4, so should definitely be compatible
- and TS is on ~4.6 at this point, so that's _really_ old
- remove the file and the declarations and declaration maps
- don't rebuild as that's usually done as a separate commit
