* refactor(test): heavily simplify the `context` helper
- since we're type-casting it anyway, we can heavily simplify this and remove the stubs entirely
- they're actually unused in the unit tests, so we don't need them at all
- besides the type-checking, which we force with a cast anyway
- the `as unknown as` is bad practice, and probably why I didn't use it initially (plus other typing issues), but it's much simpler this way and reflects the intent better -- just making it type-check with the few properties we use
- we can also use Jest mocks directly instead of the hacky `contextualLogger` and passing `data` in
- `makeContext` now creates the mocks, so we just need to check against `context.error` etc
- this is _much_ more familiar as it's what we use in the source and follows idiomatic Jest
- rewrite all the checks to test against the mocks instead
- I thought this felt too complicated / verbose before, but I had left this as is from brekk's initial test structure
- now that I understand all the tests and test intent much better, I could rewrite this to be a good bit simpler
- make the `toBeFalsy()` checks more precise by checking that the mock _wasn't_ called
- it returns `void` anyway, so `toBeFalsy()` _always_ returns true; it's not much of a test
- checking that the low verbosity level didn't trigger the mock to be called actually checks the test's intent
* make sure to check funcs in context calls too
- `get-options-overrides`'s coverage func % decreased bc funcs passed to `context.debug` weren't being called
- took me a bit to notice too since we have no coverage checks
- and then another bit to realize _why_ it decreased
- since we set a placeholder `outDir` initially, TS sets declaration map sources to that placeholder dir
- then, after ec0568ba2c8e206372f94164e697b1469bf3f33d, we remap that to the correct dir that Rollup outputs to
- this test checks that the remap happens and is correct
- 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
- use Jest's module mocks to test the error case of `tslib.ts` when `tslib` fails to import
- this took a bit of work to figure out bc, per the in-line comments, the ordering and module subpath were both _required_
- it was pretty confusing for a while until I realized what might be going wrong
- `toBeFalsy()` was a bit imprecise and always felt as such, especially since `checkTsConfig` returns `void`
- so instead check that it doesn't throw, which matches the test _intent_
- reorder the tests a bit to match existing test style: non-errors first, then errors
- and separating into two different test blocks parallelizes them as well
- also add an ES2020 check as well (follow-up to eb1dd17babde0b22e9540b12e671eb56d9d6bce0)
* test: add `parse-tsconfig` spec
- test passing tsconfig, buggy tsconfig, non-existent tsconfig, and not a tsconfig
- clean: "failed to read" error will never happen as we already checked for existence of the file earlier
- so remove the `undefined` check and instead use a non-null assertion (plus a comment explaining it)
- refactor: move the integration test for tsconfig error into this unit test instead
- faster / more efficient / more precise
- refactor: split out a `makeOptions` func that creates default plugin options to use in tests
- similar to `makeStubbedContext`
* fix windows test by normalizing
Co-authored-by: Eugene Zolenko <zolenkoe@gmail.com>
* 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
- test starting watch mode, changing a file, and adding a semantic error
- put this in a separate file as it has its own complexity to deal with (see below)
- initially put it inside `no-errors`, but it got pretty confusing; this structure is a good bit simpler
- refactored this a couple times actually
- add two watch mode helpers
- `watchBundle` is very similar to `genBundle` but with some watch mode nuances (like returning a watcher)
- `watchEnd` is a bit complicated async wrapper around a listener interface to make imperative testing simpler
- refactor: abstract out `createInput` and `createOutput` to be used in both `genBundle` and `watchBundle`
- refactor: make sure `dist` output gets put into a temp test dir
- the previous config made it output into rpt2's `dist` dir, since `cwd` is project root (when running tests from project root)
- the Rollup API's `watch` func always writes out; can't just test in-memory like with `bundle.generate`
- so the `dist` dir becomes relevant as such
- refactor: pass in a temp `testDir` instead of the `cacheRoot`
- we can derive the `cacheRoot` and the `dist` output from `testDir`
- also improve test clean-up by removing `testDir` at the end, not just the `cacheRoot` dir inside it
- `testDir` is the same var used in the unit tests to define a temp dir for testing
- change the `no-errors` fixture a tiny bit so that changing the import causes it to change too
- this ended up being fairly complex due to having to handle lots of async and parallelism
- parallel testing means fixtures have to be immutable, but watch mode needs to modify files
- ended up copying fixtures to a temp dir where I could modify them
- async events are all over here
- had to convert a callback listener interface to async too, which was pretty confusing
- and make sure the listener and bundles got properly closed too so no leaky tests
- apparently `expect.rejects.toThrow` can return a Promise too, so missed this error for a while
- refactor: make sure the error spec awaits too (though I think the errors _happen_ to throw synchronously there)
- and also found a number of bugs while attempting to test cache invalidation within watch mode
- thought it was a natural place to test since watch mode testing needs to modify files anyway
- had to trace a bunch to figure out why some code paths weren't being covered; will discuss this separately from this commit
- testing Rollup watch within Jest watch also causes Jest to re-run a few times
- I imagine double, overlapping watchers are confusing each other
- might need to disable these tests when using Jest watch
- high complexity async + parallel flows makes it pretty confusing to debug, so this code needs to be "handled with care"
- also this high complexity was even harder to debug when I'm having migraines (which is most of the time, unfortunately)
- hence why it took me a bit to finally make a PR for this small amount of code; the code was ok, the debugging was not too fun
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
- reduce the complexity of the code by decreasing the amount of nesting
- note that `print` returns `void` anyway, so calling it within the `return` statement doesn't change anything
- and this is within a `void` `forEach` at that too
- 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
- follows the style of most of the other errors in the codebase
- when referencing a variable, file path, etc, put quotes around it in the output
- so it's easier to distinguish in the log
- the docs for `clean` don't seem to have been updated with f15cb84dcc99a0bd20f3afce101c0991683010b6, which slightly changed how it works
- also clarify that "wipes out cache" means it deletes _all_ previous caches by saying "wipes any existing cache" instead
- fix the `include` default explanation to mention `.tsx` files as well, since the regex includes those
- also add code backticks and capitalize TypeScript etc (guess I missed this option in my previous PRs? or I intentionally left it as a separate change a while ago...)
- also add code backticks around `node_modules` in `cacheRoot` as well while at it
- use non-rendered newlines between sentences for markdown style consistency
- 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
- most were added in ff8895103c8466694e7d8eeb734f51d2850670d8, but these two were missed
- this should now cover all **thrown** errors
- they only get thrown in the `options` hook, which does not implement Rollup's `this.error`
- all other errors are already auto-prefixed by RollupContext etc
- per the conditional above this line, `file` is falsey, so printing it doesn't make sense
- per same conditional though, `pluginOptions.tsconfig` exists, so we can print that
- fixes a test TODO/FIXME that had to workaround this bug as well
- 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
- as the `alexlur` fork no longer exists / 404s, permalink back to og rpt
- which is archived and still exists under the `rollup` org
- as requested in issues
- integration tests and `pretty` default change got merged simultaneously, so the tests need to be updated to handle the new `pretty` default
- the coloration of the errors is quite different when using `pretty`
- so instead of re-implementing TS's colors in the tests, just test a portion that is the same color: the error text
- also TS doesn't specify semantic vs. syntax, that's something `print-diagnostics` does
- this test is more resilient to change as well I suppose since no error codes or colors
* test: add initial integration test harness
- some additional ts-jest config is needed to parse `index.ts`
- since Rollup isn't used when importing files during testing, we need to enable `esModuleInterop`
- the import of `findCacheDir` was immediately erroring without this (and maybe more, but `esModuleInterop` made the import work)
- add `tsconfig.test.json` for this purpose
- use `ts-jest` JSDoc types to get types for the ts-jest config as well
- add an integration/ dir in the __tests__/ dir for all integration tests
- add a fixtures/ dir in there as well for integration test fixtures
- add a `tsconfig.json` for all fixtures to use
- basically so that rpt2 source code is not `include`d, though also bc this may be good to customize for fixtures
- add a simple fixture with "no-errors" that does an import and a type-only import
- add a simple integration test that just ensures that all files are part of the bundle
- the bundled `index` file and each file's declaration and declaration map
- and no _other_ files!
- for Rollup, need to use paths relative to the root of the whole project
- probably because that is `cwd` when running `jest` from root
- will probably abstract out helpers here in the future as the test suite grows
- 70% coverage of `index.ts` with just this one test!
- update CONTRIBUTING.md now that an integration test exists
* refactor: use `local` func for path resolution
- similar to the unit test suite
- also remove the `.only` that I was using during development
* add comparison test for cache / no cache
- should probably do this for each integration test suite
- refactor: split out a `genBundle` func to DRY things up
- this was bound to happen eventually
- don't think testing other output formats is necessary since we don't have any specific logic for that, so that is just Rollup behavior
- take `input` path and rpt2 options as params
- and add the `tsconfig` as a default
- add a hacky workaround so that Rollup doesn't output a bunch of warnings in the logs
- format: use double quotes for strings consistently (my bad, I normally use single quotes in my own repos)
* refactor: use a temp cacheRoot
- so we can clean it up after testing to ensure consistency
- and so we're not filling up the cache in `node_modules` with testing caches
- also add a comment to one the of the tests
* test: check w/ and w/o declarations + declaration maps
- make sure they don't get output when not asked for
* fix(test): actually test the cache
- `clean: true` also means that no cache is created
- which is a bit confusing naming, but was requested by a few users and implemented in f15cb84dcc99a0bd20f3afce101c0991683010b6
- so have to create the bundle one _more_ time to create the cache
- note that `tscache`'s `getCached` and `isDirty` functions are now actually covered in the coverage report
* fix(ci): increase integration test timeout
- double it bc it was occassionally failing on CI due to timeouts
* test: ensure that JS files are part of bundle too
- ensures that TS understands the JS w/ DTS w/o error
- and that rpt2 filters out JS while Rollup still resolves it on its own (since Rollup understands ESM)
- similar to testing w/ a different plugin (i.e. literally testing an "integration"), but this is native Rollup behavior in this case where it doesn't need a plugin to understand ESM
- also the first test of actual code contents
- reformat the cache test a bit into its own block since we now have ~3 different sets of tests in the suite
* optim(test): don't need to check cache each time
- this doesn't test a new code path and the first test already tests the entire bundle for the fixture, so the other tests just repeat that
* test: add initial error checking integration tests
- refactor: rename `integration/index.spec` -> `integration/no-errors.spec`
- refactor: split `genBundle` func out into a helper file to be used by multiple integration test suites
- simplify the `genBundle` within `no-errors` as such
- create new `errors` fixture just with some simple code that doesn't type-check for now
- add test to check that the exact semantic error pops up
- and it even tests colors 😮 ...though I was confused for some time why the strings weren't matching... 😐
- add test to make sure it doesn't pop up when `check: false` or `abortOnError: false`
- when `abortOnError: false`, detect that a warning is created instead
- due to the new `errors` dir in the fixtures dir, the fixtures `tsconfig` now picks up two dirs
- which changes the `rootDir` etc
- so create two tiny `tsconfig`s in both fixture dirs that just extend that one
- and now tests all run simiarly to before
* fix(ci): increase integration test `errors` timeout
- this too timed out, probably due to the checks that _don't_ error
* optim(test): split non-erroring error tests into a different suite
- so that Jest can parallelize these
- CI was timing out on this, so splitting it out should help as it'll be 10s each
* fix(ci): bump integration test timeout to 15s
- bc it was still timing out in some cases 😕
- this time the `no-errors` suite was timing out, so just increase all around
* fix(test): output a `.js` bundle, not `.ts`
- woooops... was wondering why it was `.ts`; turns out because I wrote the `output.file` setting that way 😅😅😅
- also add a missing comment in errors for consistency
- and put code checks _after_ file structure checks, since that's a deeper check
* test: check that `emitDeclarationOnly` works as expected
- should output declaration and declaration map for file
- code + declaration should contain the one-line function
- code _shouldn't_ contain anything from TS files
- since this is plain ESM code, we don't need another plugin to process this
- nice workaround to installing another plugin that I hadn't thought of till now!
* test: add a syntactic error, refactor a bit
- add a file to `errors` with a syntax error in it
- apparently this does throw syntax err, but does not cause `emitSkipped: true`... odd...
- so may need another test for that...
- `abortOnError: false` / `check: false` both cause Rollup to error out instead
- rename `errors/index` -> `errors/semantic` since we have different kinds now
- change the `include` to only take a single file, so that we don't unnecessarily generate declarations or type-check other error files
- refactor(test): rewrite `genBundle` in both files to take a relative path to a file
- simplifies the `emitDeclarationOnly` test as we can now just reuse the local `genBundle` instead of calling `helpers.genBundle` directly
- use the same structure for `errors`'s different files as well
- refactor(test): split `errors.spec` into `tsconfig` errors, semantic errors, and syntactic errors (for now)
- add a slightly hacky workaround to get `fs.remove` to not error
- seems to be a race condition due to the thrown errors and file handles not being closed immediately on throw (seems like they close during garbage collection instead?)
- see linked issue in the comment; workaround is to just give it some more time
- not sure that there is a true fix for this, since an improper close may cause indeterminate behavior
* fix(test): normalize as arguments to `include`
- got a Windows error in CI for the `errors` test suite
- it's already been supported since eb1dd17babde0b22e9540b12e671eb56d9d6bce0, but the docs and error message were not updated to mention it
- so add both to make sure users aren't confused
- also re-order it to be ES2015, ES2020, then ESNext consistently, which is their module order (c.f. https://github.com/microsoft/TypeScript/issues/24082)
- modify test to account for the new error message
- make it a bit more resilient to change by only testing a substring as well (c.f. https://jestjs.io/docs/expect#tothrowerror)
- 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?
- 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
