Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: dogfood rpt2 as configPlugin w/ rollup.config.ts #460

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jul 18, 2023

Summary

Dogfood rpt2 more by using it as a configPlugin on our own internal rollup.config.ts

Details

Motivation

  • use rpt2 as a configPlugin within our own build process, as well as to build itself

    • doubly dogfood
  • we don't have any tests for configPlugins, and it's a Rollup CLI option, so this is one of the few ways to test it

TS Fixes

  • replace the unmaintained rollup-plugin-re w/ @rollup/plugin-replace

    • rollup-plugin-re hasn't had a commit in ~5 years and doesn't have typings (natively or in DT)
    • also import the plugins in the same order that they're used
  • fix the usage of the resolve plugin as it didn't type-check/had outdated usage

ESM Fixes

  • need to use createRequire in ESM mode

    • deps: bump @types/node to support the types for that as well
    • this is a BREAKING CHANGE as it requires Node 12+
  • fix fs-extra error due to it being CJS

  • require("package.json") -> import

    • and add resolveJsonModule to configPlugin tsconfigOverride to handle this
      • only used by the configPlugin, not everything else, so not added to tsconfig.json
  • output as build-self/index.mjs instead of build-self/dist/rollup-plugin-typescript2.es.js

    • so that types work out of the box -- due to index.d.ts
    • and so that Rollup correctly loads it as ESM and not CJS

Misc

#456 was split out of this as it took me quite a while to get builds to pass

- use rpt2 as a `configPlugin` within our own build process, as well as to build itself
  - doubly dogfood

- we don't have any tests for `configPlugin`s, and it's a Rollup CLI option, so this is one of the few ways to test it
  - create a helper `_build-self` script so we don't need to type/repeat as much (or have as much potential for mistakes)

NOTE: this commit fails to build, need to make some more adjustments to the base config to get it to work in TS
- will do that in the next commits so that it's easier to read/differentiate/review
- replace the unmaintained `rollup-plugin-re` w/ `@rollup/plugin-replace`
  - `rollup-plugin-re` hasn't had a commit in ~5 years and doesn't have typings (natively or in DT)
  - also import the plugins in the same order that they're used
- `require("package.json")` -> `import`
  - and add `resolveJsonModule` to `configPlugin` `tsconfigOverride` to handle this
    - only used by the `configPlugin`, not everything else, so not added to `tsconfig.json`

- fix the usage of the `resolve` plugin as it didn't type-check/had outdated usage
- need to use `createRequire` in ESM mode
  - deps: bump `@types/node` to support the types for that as well
  - this is a BREAKING CHANGE as it requires Node 14+
- fix `fs-extra` error due to it being CJS

- add `build-self/*` to `prebuild` clean
  - not to `build-self` script, as the `build-self/` directory has to exist for that
- output as `build-self/index.mjs` instead of `build-self/dist/rollup-plugin-typescript2.es.js`
  - so that types work out of the box -- due to `index.d.ts`
  - and so that Rollup correctly loads it as ESM and not CJS
@agilgur5 agilgur5 added kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs version: minor Increment the minor version when merged scope: tests Tests could be improved. Or changes that only affect tests scope: configPlugin Related to usage as a configPlugin for reading rollup.config.ts (vs. regular plugin) labels Jul 18, 2023
@agilgur5
Copy link
Collaborator Author

As this is a breaking change due to use of createRequire, should probably go out in the same release as #458

- set `engines.node` in `package.json` to show this as well
@agilgur5
Copy link
Collaborator Author

Tests are failing due to use of createRequire... this might require bumping ts-jest 😕

@ezolenko
Copy link
Owner

ezolenko commented Jul 30, 2023

I'm also getting an error on Windows trying one of the build_self scripts, a rollup issue?

> rollup -c rollup.config.self.ts --configPlugin ./build-self/index.mjs="{ tsconfigOverride: { compilerOptions: { resolveJsonModule: true } } }"

[!] Error: Cannot load plugin "C:\sandbox\other\rpt2\rollup-plugin-typescript2\build-self\index.mjs": Only URLs with a scheme in: file and data are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'.
Error: Cannot load plugin "C:\sandbox\other\rpt2\rollup-plugin-typescript2\build-self\index.mjs": Only URLs with a scheme in: file and data are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'.
    at loadAndRegisterPlugin (C:\sandbox\other\rpt2\rollup-plugin-typescript2\node_modules\rollup\dist\shared\loadConfigFile.js:508:23)
    at addPluginsFromCommandOption (C:\sandbox\other\rpt2\rollup-plugin-typescript2\node_modules\rollup\dist\shared\loadConfigFile.js:458:17)
    at getDefaultFromTranspiledConfigFile (C:\sandbox\other\rpt2\rollup-plugin-typescript2\node_modules\rollup\dist\shared\loadConfigFile.js:583:5)
    at loadConfigFile (C:\sandbox\other\rpt2\rollup-plugin-typescript2\node_modules\rollup\dist\shared\loadConfigFile.js:565:11)
    at Object.loadAndParseConfigFile (C:\sandbox\other\rpt2\rollup-plugin-typescript2\node_modules\rollup\dist\shared\loadConfigFile.js:545:21)
    at getConfigs (C:\sandbox\other\rpt2\rollup-plugin-typescript2\node_modules\rollup\dist\bin\rollup:1691:39)
    at runRollup (C:\sandbox\other\rpt2\rollup-plugin-typescript2\node_modules\rollup\dist\bin\rollup:1665:43)

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 30, 2023

Dang thought we were past all Windows pathing issues 😅

Looks like something is expanding/converting the path in a way Rollup doesn't like ./build-self/index.mjs -> C:\sandbox\other\rpt2\rollup-plugin-typescript2\build-self\index.mjs.
I'm not sure if Rollup is doing that expansion or something else (NPM as it's a script?), but I would think Rollup should know how to interpret that correctly 🤔 The error message makes it sound like potentially an ESM spec issue. So if Rollup is the one expanding it, it's expanding incorrectly for ESM. If it's NPM or something else expanding, that's a bit more of a head-scratcher

@agilgur5 agilgur5 marked this pull request as draft September 28, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: configPlugin Related to usage as a configPlugin for reading rollup.config.ts (vs. regular plugin) scope: tests Tests could be improved. Or changes that only affect tests version: minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants