chore: build and export types [TCTC-3320] #1455

ninofiliu · 2022-08-19T15:28:38Z

TL;DR

https://toucantoco.atlassian.net/browse/TCTC-3320
https://github.com/ToucanToco/tucana/pull/12553

Weaverbird is fully written in TS but didn't export type definitions, which frequently broke Tucana and other dependent packages. This PR adds a build script to generate type definitions to resolve this.

Before:

❌ no type defs, usual workaround in Tucana was to ts-ignore imports

❌ type errors not prevented

After:

✔️ type defs of imports

✔️ error prevention

How

I first wanted to use rollup-plugin-dts, but it turned out that it required the latest rollup, the latest version of rollup plugins, the latest typescript, and after all these updates and many fixes in the code itself, the build was broken, likely due to a rollup error

nino@dell~/tc/weaverbirdchore/dts$ yarn run build
yarn run v1.22.19
$ yarn build-bundle
$ rollup -c
clean: postcss.plugin was deprecated. Migration guide:
https://evilmartians.com/chronicles/postcss-8-plugin-migration

src/main.ts → dist/weaverbird.common.js, dist/weaverbird.esm.js, dist/weaverbird.browser.js...
(!) Plugin commonjs: The namedExports option from "@rollup/plugin-commonjs" is deprecated. Named exports are now handled automatically.
(!) Plugin replace: @rollup/plugin-replace: 'preventAssignment' currently defaults to false. It is recommended to set this option to `true`, as the next major version will default this option to `true`.
[!] Error: Unexpected character '@' (Note that you need plugins to import files that are not JavaScript)
src/components/Pagination.vue?rollup-plugin-vue=script.ts (29:0)
27: import { numberOfPages, pageMinMax, PaginationContext } from '@/lib/dataset/pagination';
28: 
29: @Component({
    ^
30:   name: 'pagination',
31:   components: {
Error: Unexpected character '@' (Note that you need plugins to import files that are not JavaScript)
    at error (/home/nino/tc/weaverbird/node_modules/rollup/dist/shared/rollup.js:198:30)
    at Module.error (/home/nino/tc/weaverbird/node_modules/rollup/dist/shared/rollup.js:12559:16)
    at Module.tryParse (/home/nino/tc/weaverbird/node_modules/rollup/dist/shared/rollup.js:12936:25)
    at Module.setSource (/home/nino/tc/weaverbird/node_modules/rollup/dist/shared/rollup.js:12841:24)
    at ModuleLoader.addModuleSource (/home/nino/tc/weaverbird/node_modules/rollup/dist/shared/rollup.js:22283:20)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I tried to fix this error but it looked like a messy thing to fix because things were supposed to work already: this is rollup failing to transpile a typescript file with decorators, eventhough decorator transpilation worked before all the updates, and went until page 5 of google to find answers and none worked.

I also tried to update plugin versions not to their latest, but I either got the error above or heap overflow errors. At this point I abandonned.

All wasn't lost tho, the build broke only on the last commit, so there's this free work available if we want weaverbird to be built with the latest typescript and rollup version (but not with the latest plugin versions apparently).

The branch is chore/dts if you wanna take a look.

Why the build script? Why not use TSC directly to generate types?

If we generate types they'll still have path aliases in them. Stuff that looks like:

// /dist/types/main.d.ts
import foo from '@/bar/baz/foo';

and these path won't resolve when being used in dependent modules. Everybody wants TSC to natively resolve paths aliases when building declaration files microsoft/TypeScript#15479 microsoft/TypeScript#5039 but it has been refused several times so I figured that having a dirty but simple sed-based script to replace '@' by the path to src in declaration files was the simplest workaround.

Should we worry about the impact of this?

Probably not.

Build time time only lasts 7% longer.

nino@dell~/tc/weaverbirdmaster$ time npx rollup -c
[...]
real	0m56.346s
user	1m25.636s
sys	0m4.164s
nino@dell~/tc/weaverbirdchore/dts2$ time . build-types.sh 
real    0m3.966s
user	0m7.545s
sys	0m0.278s

The only thing I'd probably worry a tiny bit about would be the use of the dot command but it's a POSIX standard so should work on mac on unix, and also works in powershell.

render · 2022-08-19T15:28:44Z

Your Render PR Server URL is https://weaverbird-playground-pr-1455.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cbvqoa2rrk0947jip890.

codecov · 2022-08-19T15:34:45Z

Codecov Report

Merging #1455 (8c3a1c4) into master (344f55b) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 8c3a1c4 differs from pull request most recent head 99f3df8. Consider uploading reports for the commit 99f3df8 to get more accurate results

@@            Coverage Diff             @@
##           master    #1455      +/-   ##
==========================================
- Coverage   98.47%   98.45%   -0.02%     
==========================================
  Files         212      213       +1     
  Lines        6083     6084       +1     
  Branches      956      956              
==========================================
  Hits         5990     5990              
- Misses         93       94       +1

Impacted Files	Coverage Δ
weaverbird/src/types.ts	`0.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ninofiliu · 2022-08-19T15:56:41Z

ah shit i forgot about tests i'm on it yo

ninofiliu · 2022-08-19T16:43:09Z

CI fails with

55: export { PipelineStep, Pipeline } from './lib/steps';
             ^
Error: 'PipelineStep' is not exported by src/lib/steps.ts

but that's baloney, I don't know what's happening, I'm rebasing rn

ninofiliu · 2022-08-19T18:36:39Z

Oooooh okay that was a hard one

The docs say that it comes from the commonjs plugin not inferring the right named exports, so I tried every combination of

// rollup.config.js
export default {
  // ...
  plugins: [
    // ...
    commonjs({
      namedExports: {
        'src/lib/steps.ts': ['PipelineStep', 'Pipeline'],
        // ...
      },
    });
 ]
};

with and without the ts, with relative path, with full path, with the path as it appears in the file, etc, but to no avail. I upgraded the plugin to latest also, still the same error. After a while I gave up because I didn't expect that much from a deprecated plugin.

Instead I decided to have a specific file for the type defs src/types.ts so that the main rollup pipeline should not even worry about these types, so now that should be good

ninofiliu · 2022-08-19T18:37:54Z

OH MY GOD SONAR CAN YOU BE NOT STUPID FOR LIKE HALF A SECOND AND NOT CHECK THE CODE COVERAGE OF A FILE THAT JUST PASSES EXPORTS AROUND

ninofiliu · 2022-08-19T19:10:21Z

FINALLY the CI is green, god that was exhausting

davinov

Great!
Thanks for your research and effort.
Indeed, we feel kinda stuck with today's build, which seems very very difficult to update :/

I think we'll take the opportunity of the rewrite in React of weaverbird's front-end to renew this build completely.
In the meantime, what you suggest is awesome and solves exactly the problem we had! Congrats 👏

davinov · 2022-08-22T13:16:24Z

build-types.sh

@@ -0,0 +1,13 @@
+#!/bin/sh
+rm -rf dist/types
+npx tsc -p tsconfig.types.json


Should we add npx to dev devpendencies then?
We're lucky this is provided in the node image used by the CI ^^
Otherwise, calling tsc directly could be enough? (because this script is called from npm run ... which adds node_modules/.bin to PATH (https://docs.npmjs.com/cli/v8/commands/npm-run-script).

Wait, isn't npx available wherever node is? If not I don't have any preferences. Maybe the dot command erases the path? Not sure. Probably the safer thing would be to replace npx tsc by node node_modules/.bin/tsc then?

Okay so apparently npx is available wherever npm is and I doubt that it's necessary to handle build systems where npm is not available yet yarn is, I can't even think of one

Oh nice! I thought it was standalone (https://www.npmjs.com/package/npx)
Good to use npx then!

davinov · 2022-08-22T13:19:10Z

build-types.sh

+    path_to_src=$(echo $file | tr -dc '/' | colrm 1 2 | sed "s#/#../#g")
+  fi
+  sed -i -r "s#((import|export) .* from ')@/(.*)#\1$path_to_src\3#g" $file
+done


Another approach could have been to use a codemod like this one:https://github.com/tusharmath/ts-codemod/blob/master/transformations/normalize-import-path.ts
However, it does seem simple enough for now, so the bash script can be as reliable and handle all cases (1 for now :p)

Didn't know about this one! Yeah let's switch to AST-based codemods if this build pipeline get too complex

ninofiliu added 2 commits August 19, 2022 16:08

chore: add build-types script

6f04298

chore: use relative paths when possible in main.ts

0dd7a32

ninofiliu added enhancement New feature or request need review tech technical tasks build proposal labels Aug 19, 2022

ninofiliu self-assigned this Aug 19, 2022

ninofiliu added 2 commits August 19, 2022 18:39

chore: export types used in tucana

ab0ccd0

chore: minor version bump

ab9fc0f

ninofiliu force-pushed the chore/dts2 branch from 94f2a3d to ab9fc0f Compare August 19, 2022 16:42

ninofiliu added 2 commits August 19, 2022 20:05

chore: lint rollup config

5c283f7

chore: export types from types.ts

77730e4

ninofiliu force-pushed the chore/dts2 branch from 7a0e8e0 to 77730e4 Compare August 19, 2022 18:28

chore: ignore cov on src/types.ts

99f3df8

ninofiliu force-pushed the chore/dts2 branch from 8c3a1c4 to 99f3df8 Compare August 19, 2022 18:58

ninofiliu requested review from alice-sevin and davinov August 22, 2022 07:34

davinov approved these changes Aug 22, 2022

View reviewed changes

ninofiliu merged commit 180b427 into master Aug 22, 2022

ninofiliu deleted the chore/dts2 branch August 22, 2022 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: build and export types [TCTC-3320] #1455

chore: build and export types [TCTC-3320] #1455

ninofiliu commented Aug 19, 2022 •

edited

Loading

render bot commented Aug 19, 2022

codecov bot commented Aug 19, 2022 •

edited

Loading

ninofiliu commented Aug 19, 2022

ninofiliu commented Aug 19, 2022

ninofiliu commented Aug 19, 2022

ninofiliu commented Aug 19, 2022 •

edited

Loading

ninofiliu commented Aug 19, 2022

davinov left a comment

davinov Aug 22, 2022

ninofiliu Aug 22, 2022

ninofiliu Aug 22, 2022 •

edited

Loading

davinov Aug 22, 2022

davinov Aug 22, 2022

ninofiliu Aug 22, 2022

chore: build and export types [TCTC-3320] #1455

chore: build and export types [TCTC-3320] #1455

Conversation

ninofiliu commented Aug 19, 2022 • edited Loading

TL;DR

How

Why the build script? Why not use TSC directly to generate types?

Should we worry about the impact of this?

render bot commented Aug 19, 2022

codecov bot commented Aug 19, 2022 • edited Loading

Codecov Report

ninofiliu commented Aug 19, 2022

ninofiliu commented Aug 19, 2022

ninofiliu commented Aug 19, 2022

ninofiliu commented Aug 19, 2022 • edited Loading

ninofiliu commented Aug 19, 2022

davinov left a comment

Choose a reason for hiding this comment

davinov Aug 22, 2022

Choose a reason for hiding this comment

ninofiliu Aug 22, 2022

Choose a reason for hiding this comment

ninofiliu Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

davinov Aug 22, 2022

Choose a reason for hiding this comment

davinov Aug 22, 2022

Choose a reason for hiding this comment

ninofiliu Aug 22, 2022

Choose a reason for hiding this comment

ninofiliu commented Aug 19, 2022 •

edited

Loading

codecov bot commented Aug 19, 2022 •

edited

Loading

ninofiliu commented Aug 19, 2022 •

edited

Loading

ninofiliu Aug 22, 2022 •

edited

Loading