- the only reason that `ConsoleContext` was still used was because the `options` hook doesn't support certain context functions like `this.error` / `this.warn` etc
- but `buildStart` does! so we can just move pretty much everything into `buildStart` instead
- didn't move setting `rollupOptions` because that value gets hashed and the value in `buildStart` is different, as it is after all plugins' `options` hooks have ran
- this way we don't need `ConsoleContext` what-so-ever and so can remove it entirely!
- as well as `IContext`, its tests, etc
- use `this.error` instead of `throw`ing errors in `parse-tsconfig` as it exists now
- couldn't in the `options` hook, can in `buildStart`
- this also ensure we don't have all the `rpt2: ` prefixes ever missing again
- c.f. ff8895103c8466694e7d8eeb734f51d2850670d8, 0628482ea26ddf56e6ef5521e4dcb85ef5b4beb6
- refactor `parse-tsconfig.spec` to account for this change
- c.f. Rollup hooks docs: https://rollupjs.org/guide/en/#options, https://rollupjs.org/guide/en/#buildstart
- when `options` is called, `generateRound` should have already been reset to `0`, so this is redundant / unnecessary
- `options` is only called once per watch cycle
- `options` is also an `input` hook, not an `output` hook, i.e. it's only called once for _all_ outputs, _not_ per each output
- `generateRound` only tracks the output round
- this might be leftover historical remnants prior to Rollup officially separating `output` hooks, but even before then, it should only have been called once for _all_ outputs
- since, per the Rollup API, it's only called during `rollup.rollup` and not during `bundle.generate` etc
- c.f. old docs https://github.com/rollup/rollup/blob/v1.18.0/docs/05-plugin-development.md#options
- it's possible this is _even older_, but I couldn't find plugin docs for Rollup pre-1.0 to try to confirm against
* refactor: combine two context files into one
- they're virtually identical, so combine them instead of keeping them separate
- changes to one should probably be made to both
- still a < 100 LoC file
- refactor out `_.isFunction` with a simple `getText` function instead
- checks the opposite
- one more lodash removal!
- add docstrings about when to use the two contexts
* disable tslint rule for this file
This reverts commit e145d0baebbbef1eec691b99d1ce0dce1e49f38c.
- Per discussion on the original issue and reverted PR, it seems that the request to use `.d.ts` instead of `.vue.d.ts` was made in error
- `.d.ts` seems to only be necessary if Vue users were importing `.vue` SFCs without extensions ("extensionless")
- i.e. `import MyComponent from "./MyComponent"` instead of `import MyComponent from "./MyComponent.vue"`
- and "extensionless" imports are no longer supported by the Vue team (but used to be)
- requiring extensionless imports also breaks imports when extensions _are_ used
- so these are not necessarily compatible with each other, but the Vue team support strongly suggests that `.vue.d.ts` would be the proper way forward
- just slightly simplify / DRY up repeated code
- this matches the style of some of the other functions in the codebase, such as `emitDeclaration`, `getAllReferences`, etc
- also fix indentation in `typecheckFile`
- apparently I double-intended this accidentally, so make it single indent
- since the `buildEnd` hook exists nowadays, we can just reset it there, right before `generateBundle` is called for each output and increments it per output
- also modify the comment to account for this change and the fact that `buildEnd` exists and is used nowadays
- we can re-use the same one during watch instead of resetting it in the `options` hook
- should make the LS a bit faster / more efficient in watch mode as most source files are shared between watch cycles
- a few issues have reported that "failed to transpile" is a vague / confusing error
- and `emitSkipped` actually _doesn't_ mean that it failed to transpile, as, in current versions of TS, it's not due to syntactic or semantic errors
- so explicitly say "Emit skipped" instead, which is slightly less vague, in that it can actually be used as a search term
- and provide a link to my TS issue that lists some reasons why `emitSkipped` occurs, since this is unfortunately otherwise undocumented by TS
- in the future, hopefully that issue will be resolved and we'll be able to give better or more specific error messages, but for now this is probably the best we can do due to its undocumented nature, unfortunately
- gotta remember to normalize file names 🙃
- this fixes a failing test on Windows by fixing the underlying bug
- refactor: improve normalization in errors.spec
- normalize _all_ calls to `local`, instead of just one
- this doesn't affect the tests, only the source code change does, but I added this first and it's better practice / test future-proofing
* fix: type-check `include`d files missed by transform (type-only files)
- type-only files never get processed by Rollup as their imports are
elided/removed by TS in the resulting compiled JS file
- so, as a workaround, make sure that all files in the `tsconfig`
`include` (i.e. in `parsedConfig.fileNames`) are also type-checked
- note that this will not catch _all_ type-only imports, in
particular if one is using `tsconfig` `files` (or in general _not_
using glob patterns in `include`) -- this is just a workaround,
that requires a separate fix
- we do the same process for generating declarations for "missed"
files right now in `_onwrite`, so basically do the same thing but
for type-checking in `_ongenerate`
(_technically_ speaking, there could be full TS files
(i.e. _not_ type-only) that are in the `include` and weren't
transformed
- these would basically be independent TS files not part of the bundle
that the user wanted type-checking and declarations for (as we
_already_ generate declarations for those files))
* move misssed type-checking to `buildEnd` hook, remove declarations check
- `buildEnd` is a more correct place for it, since this does not generate any output files
- (unlike the missed declarations)
- and this means it's only called once per build, vs. once per output
- remove the check against the `declarations` dict as one can type-check without outputting declarations
- i.e. if `declaration: false`; not the most common use-case for rpt2, but it is one
* add new checkedFiles Set to not duplicate type-checking
- since `parsedConfig.fileNames` could include files that were already checked during the `transform` hook
- and because `declarations` dict is empty when `declaration: false`, so can't check against that
* move checkedFiles.add to the beginning of typecheckFile
- because printing diagnostics can bail if the category was error
- that can result in a file being type-checked but not added to checkedFiles
* wip: fuse _ongenerate functionality into buildEnd, _onwrite into generateBundle
- per ezolenko, the whole generateRound etc stuff was a workaround because the buildEnd hook actually _didn't exist_ before
- so now that it does, we can use it to simplify some logic
- no longer need `_ongenerate` as that should be in `buildEnd`, and no longer need `_onwrite` as it is the only thing called in `generateBundle`, so just combine them
- importantly, `buildEnd` also occurs before output generation, so this ensures that type-checking still occurs even if `bundle.generate()` is not called
- also move the `walkTree` call to above the "missed" type-checking as it needs to come first
- it does unconditional type-checking once per watch cycle, whereas "missed" only type-checks those that were, well, "missed"
- so in order to not have duplication, make "missed" come after, when the `checkedFiles` Set has been filled by `walkTree` already
- and for simplification, just return early on error to match the current behavior
- in the future, may want to print the error and continue type-checking other files
- so that all type-check errors are reported, not just the first one
NOTE: this is WIP because the `cache.done()` call and the `!noErrors` message are incorrectly blocked behind the `pluginOptions.check` conditional right now
- `cache.done()` needs to be called regardless of check or error or not, i.e. in all scenarios
- but ideally it should be called after all the type-checking here
- `!noErrors` should be logged regardless of check or not
- and similarly, after the type-checking
* call `cache().done()` and `!noErrors` in check and non-check conditions
- instead of making a big `if` statement, decided to split out a `buildDone` function
- to always call at the end of the input phase
- we can also move the `cache().done()` in `emitSkipped` into `buildEnd`, as `buildEnd` gets called when an error occurs as well
- and this way we properly print for errors as well
- `buildDone` will have more usage in other PRs as well, so I figure it makes sense to split it out now as well
* use `RollupContext` for type-only files
- i.e. bail out when `abortOnError: true`, which `ConsoleContext` can't do
- `ConsoleContext` is basically meant for everywhere `RollupContext` can't be used
- which is effectively only in the `options` hook, per the Rollup docs: https://rollupjs.org/guide/en/#options
* add test for type-only file with type errors
- now that the integration tests exist, we can actually test this scenario
- refactor: give each test their own `onwarn` mock when necessary
- while `restoreMocks` is set in the `jest.config.js`, Jest apparently has poor isolation of mocks: https://github.com/facebook/jest/issues/7136
- if two tests ran in parallel, `onwarn` was getting results from both, screwing up the `toBeCalledTimes` number
- couldn't get the syntax error to work with `toBeCalledTimes` either
- if no mock is given, it _does_ print warnings, but if a mock is given, it doesn't print, yet isn't called?
- I think the mock is getting screwed up by the error being thrown here; maybe improperly saved or something
- `process.env.ROLLUP_WATCH` is only set when using the CLI
- when using watch mode through the Rollup API (https://rollupjs.org/guide/en/#rollupwatch), this is not set
- but since Rollup 2.14.0 (https://github.com/rollup/rollup/blob/master/CHANGELOG.md#2140), `this.meta.watchMode` is available
- still check the env variable for backward-compat on older Rollup versions
- notably, this is needed to test watch mode, since the tests are done via the Rollup API
- apparently `emitSkipped` can indeed happen without there being a semantic or syntactic error
- so add back the previous unconditional behavior of `noErrors`
- still not clear what exactly triggers `emitSkipped` though, see my TS issue
- we require Rollup `>=1.26.3` in the peerDeps and the README and have for years now
- (and Rollup is currently at `2.75.7`, so this is a pretty low target)
- as such, we can remove various checks that are legacy backward-compat remnants for old Rollup versions:
- `this.meta` was added to `options` in `1.1.0`: https://github.com/rollup/rollup/blob/master/CHANGELOG.md#110
- `this.addWatchFile` was added at least in `1.0.0`: https://github.com/rollup/rollup/blob/master/CHANGELOG.md#100
- `this.error` and `this.warn` were added to `transform` in `0.41.0`: https://github.com/rollup/rollup/blob/master/CHANGELOG.md#0410
- this simplifies some of the code a decent bit, `RollupContext` in particular
- see also the usage of `buildEnd` in my other PR
- modify tests to account for these changes; basically just simplify them
- and remove the check against private internals of the class, as they're private
- guess I forgot to remove this when I was refactoring the test code?
* 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
* 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
- 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
- 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>
- _.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
- 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
- this was introduced in v4.1.0 of @rollup/pluginutils:
https://github.com/rollup/plugins/blob/master/packages/pluginutils/CHANGELOG.md#v410
- this is the same as the code in `normalize.ts` but it uses constants
from Node and is used by multiple Rollup plugins, so just helps with
standardization
- also less code and types to ship in the bundle!
- removed the dist files for `normalize` as well, but didn't do a build
in this commit as those are usually done in separate commits
On Windows the normalized paths in resolveId end up in POSIX format.
This cause rollup to treat the returned path as a new piece of content.
This in turn results in duplicate output for references across entry points.
Fixed by normalizing the path to use host OS separators before returning.
- previously, declarationDir was set to cwd if useTsconfigDeclarationDir
wasn't true, however, declarations aren't output to cwd, but to
Rollup's output destination, so this was incorrect
- instead, don't set declarationDir, which defaults it to outDir,
which is currently set to a placeholder
- previously, it rewrote declarations to output to Rollup's dest
from cwd, now rewrite from outDir placeholder instead
- and add a rewrite of sources to match relative path from Rollup's
output dest instead of outDir placeholder
- also change the one line in the docs that says it'll be
`process.cwd()`; every other reference says it'll be the output dest
