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

Stop copying test files to es and lib #2320

Closed
becca-bailey opened this issue Jun 21, 2022 · 5 comments · Fixed by #2321
Closed

Stop copying test files to es and lib #2320

becca-bailey opened this issue Jun 21, 2022 · 5 comments · Fixed by #2321
Labels
Type: Infrastructure 🏠 Requested changes that won't affect core functionality

Comments

@becca-bailey
Copy link
Contributor

As a part of #2301, we moved our tests to the directories they are testing. This required a few config updates in order to get everything compiling correctly.

However, we ran into a conflict between tests and typescript. In order to package our generated type files with Victory, we are using the --copy-files flag in our babel CLI commands. This takes all files that have not been transformed with babel and copies them to the es and lib packaged output directories.

We have some ability to configure which files are copied by ignoring certain files in the babel build process and using the --no-copy-ignored flag. However, .d.ts files are also ignored by babel, so this doesn't work for our use case.

As a temporary measure, we are copying all files, including test files to the compiled output, and we have set up both jest and npm to ignore these copied test files.

Eventually, we should make sure these test files are not copied at all. This might involve some more advanced babel configuration, using another build tool, or removing our dependency on babel altogether once the typescript conversion is complete.

@becca-bailey becca-bailey added the Type: Infrastructure 🏠 Requested changes that won't affect core functionality label Jun 21, 2022
@ryan-roemer
Copy link
Member

I think this was fixed after move-tests was merged to main.

For a clean build in a JS-based project and TS-based project:

$ lerna exec --scope victory-area -- yarn nps build-libs && tree packages/victory-area/{es,lib}
# ...
lerna success exec Executed command in 1 package: "yarn nps build-libs"
packages/victory-area/es
├── area.js
├── helper-methods.js
├── index.d.ts
├── index.js
└── victory-area.js
packages/victory-area/lib
├── area.js
├── helper-methods.js
├── index.d.ts
├── index.js
└── victory-area.js

0 directories, 10 files

and

$ lerna exec --scope victory-core -- yarn nps build-libs && tree packages/victory-core/{es,lib}
# ...
lerna success exec Executed command in 1 package: "yarn nps build-libs"
packages/victory-core/es
├── index.d.ts
├── index.js
├── types
│   ├── callbacks.d.ts
│   ├── callbacks.js
│   ├── prop-types.d.ts
│   └── prop-types.js
├── victory-accessible-group
│   ├── victory-accessible-group.d.ts
│   └── victory-accessible-group.js
├── victory-animation
│   ├── util.d.ts
│   ├── util.js
│   ├── victory-animation.d.ts
│   └── victory-animation.js
├── victory-clip-container
│   ├── victory-clip-container.d.ts
│   └── victory-clip-container.js
├── victory-container
│   ├── victory-container.d.ts
│   └── victory-container.js
├── victory-label
│   ├── victory-label.d.ts
│   └── victory-label.js
├── victory-portal
│   ├── portal-context.d.ts
│   ├── portal-context.js
│   ├── portal.d.ts
│   ├── portal.js
│   ├── victory-portal.d.ts
│   └── victory-portal.js
├── victory-primitives
│   ├── arc.d.ts
│   ├── arc.js
│   ├── background.d.ts
│   ├── background.js
│   ├── border.d.ts
│   ├── border.js
│   ├── circle.d.ts
│   ├── circle.js
│   ├── clip-path.d.ts
│   ├── clip-path.js
│   ├── index.d.ts
│   ├── index.js
│   ├── line-segment.d.ts
│   ├── line-segment.js
│   ├── line.d.ts
│   ├── line.js
│   ├── path.d.ts
│   ├── path.js
│   ├── point.d.ts
│   ├── point.js
│   ├── rect.d.ts
│   ├── rect.js
│   ├── text.d.ts
│   ├── text.js
│   ├── tspan.d.ts
│   ├── tspan.js
│   ├── types.d.ts
│   ├── types.js
│   ├── whisker.d.ts
│   └── whisker.js
├── victory-theme
│   ├── grayscale.d.ts
│   ├── grayscale.js
│   ├── material.d.ts
│   ├── material.js
│   ├── types.d.ts
│   ├── types.js
│   ├── victory-theme.d.ts
│   └── victory-theme.js
├── victory-transition
│   ├── victory-transition.d.ts
│   └── victory-transition.js
└── victory-util
    ├── add-events.js
    ├── axis.d.ts
    ├── axis.js
    ├── collection.d.ts
    ├── collection.js
    ├── common-props.d.ts
    ├── common-props.js
    ├── data.d.ts
    ├── data.js
    ├── default-transitions.d.ts
    ├── default-transitions.js
    ├── domain.d.ts
    ├── domain.js
    ├── events.js
    ├── helpers.d.ts
    ├── helpers.js
    ├── hooks
    │   ├── index.js
    │   ├── use-animation-state.js
    │   └── use-previous-props.js
    ├── immutable.d.ts
    ├── immutable.js
    ├── index.d.ts
    ├── index.js
    ├── label-helpers.d.ts
    ├── label-helpers.js
    ├── line-helpers.d.ts
    ├── line-helpers.js
    ├── log.d.ts
    ├── log.js
    ├── point-path-helpers.d.ts
    ├── point-path-helpers.js
    ├── prop-types.js
    ├── scale.d.ts
    ├── scale.js
    ├── selection.d.ts
    ├── selection.js
    ├── style.d.ts
    ├── style.js
    ├── textsize.d.ts
    ├── textsize.js
    ├── timer-context.d.ts
    ├── timer-context.js
    ├── timer.d.ts
    ├── timer.js
    ├── transitions.d.ts
    ├── transitions.js
    ├── user-props.d.ts
    ├── user-props.js
    ├── wrapper.d.ts
    └── wrapper.js
packages/victory-core/lib
├── index.d.ts
├── index.js
├── types
│   ├── callbacks.d.ts
│   ├── callbacks.js
│   ├── prop-types.d.ts
│   └── prop-types.js
├── victory-accessible-group
│   ├── victory-accessible-group.d.ts
│   └── victory-accessible-group.js
├── victory-animation
│   ├── util.d.ts
│   ├── util.js
│   ├── victory-animation.d.ts
│   └── victory-animation.js
├── victory-clip-container
│   ├── victory-clip-container.d.ts
│   └── victory-clip-container.js
├── victory-container
│   ├── victory-container.d.ts
│   └── victory-container.js
├── victory-label
│   ├── victory-label.d.ts
│   └── victory-label.js
├── victory-portal
│   ├── portal-context.d.ts
│   ├── portal-context.js
│   ├── portal.d.ts
│   ├── portal.js
│   ├── victory-portal.d.ts
│   └── victory-portal.js
├── victory-primitives
│   ├── arc.d.ts
│   ├── arc.js
│   ├── background.d.ts
│   ├── background.js
│   ├── border.d.ts
│   ├── border.js
│   ├── circle.d.ts
│   ├── circle.js
│   ├── clip-path.d.ts
│   ├── clip-path.js
│   ├── index.d.ts
│   ├── index.js
│   ├── line-segment.d.ts
│   ├── line-segment.js
│   ├── line.d.ts
│   ├── line.js
│   ├── path.d.ts
│   ├── path.js
│   ├── point.d.ts
│   ├── point.js
│   ├── rect.d.ts
│   ├── rect.js
│   ├── text.d.ts
│   ├── text.js
│   ├── tspan.d.ts
│   ├── tspan.js
│   ├── types.d.ts
│   ├── types.js
│   ├── whisker.d.ts
│   └── whisker.js
├── victory-theme
│   ├── grayscale.d.ts
│   ├── grayscale.js
│   ├── material.d.ts
│   ├── material.js
│   ├── types.d.ts
│   ├── types.js
│   ├── victory-theme.d.ts
│   └── victory-theme.js
├── victory-transition
│   ├── victory-transition.d.ts
│   └── victory-transition.js
└── victory-util
    ├── add-events.js
    ├── axis.d.ts
    ├── axis.js
    ├── collection.d.ts
    ├── collection.js
    ├── common-props.d.ts
    ├── common-props.js
    ├── data.d.ts
    ├── data.js
    ├── default-transitions.d.ts
    ├── default-transitions.js
    ├── domain.d.ts
    ├── domain.js
    ├── events.js
    ├── helpers.d.ts
    ├── helpers.js
    ├── hooks
    │   ├── index.js
    │   ├── use-animation-state.js
    │   └── use-previous-props.js
    ├── immutable.d.ts
    ├── immutable.js
    ├── index.d.ts
    ├── index.js
    ├── label-helpers.d.ts
    ├── label-helpers.js
    ├── line-helpers.d.ts
    ├── line-helpers.js
    ├── log.d.ts
    ├── log.js
    ├── point-path-helpers.d.ts
    ├── point-path-helpers.js
    ├── prop-types.js
    ├── scale.d.ts
    ├── scale.js
    ├── selection.d.ts
    ├── selection.js
    ├── style.d.ts
    ├── style.js
    ├── textsize.d.ts
    ├── textsize.js
    ├── timer-context.d.ts
    ├── timer-context.js
    ├── timer.d.ts
    ├── timer.js
    ├── transitions.d.ts
    ├── transitions.js
    ├── user-props.d.ts
    ├── user-props.js
    ├── wrapper.d.ts
    └── wrapper.js

24 directories, 228 files

@becca-bailey
Copy link
Contributor Author

So, I'm still seeing test files show up in lib and es when we run yarn nps watch. This isn't a big deal since this particular command is only being run during local development, so I can close this issue for now. But it might be something to try to fix if we don't want to ignore built test files when we run tests during development.

@ryan-roemer
Copy link
Member

Ah! I think we're missing:

diff --git a/package-scripts.js b/package-scripts.js
index f418c271..afc5ccd5 100644
--- a/package-scripts.js
+++ b/package-scripts.js
@@ -134,9 +134,9 @@ module.exports = {
       "lerna version --no-git-tag-version --no-push --loglevel silly",
     // TODO: organize build scripts once build perf is sorted out
     "babel-es":
-      "cross-env BABEL_ENV=es babel src --out-dir es --config-file ../../.babelrc.js --extensions .tsx,.ts,.jsx,.js --source-maps",
+      "cross-env BABEL_ENV=es babel src --out-dir es --config-file ../../.babelrc.build.js --extensions .tsx,.ts,.jsx,.js --source-maps",
     "babel-lib":
-      "cross-env BABEL_ENV=commonjs babel src --out-dir lib --config-file ../../.babelrc.js --extensions .tsx,.ts,.jsx,.js --source-maps",
+      "cross-env BABEL_ENV=commonjs babel src --out-dir lib --config-file ../../.babelrc.build.js --extensions .tsx,.ts,.jsx,.js --source-maps",
     "babel-es-watch": "nps babel-es -- -- --watch",
     "babel-lib-watch": "nps babel-lib -- -- --watch",
     "build-es": npsUtils.series.nps("clean.es", "babel-es", "types.es"),

I'm testing that out right now.

Maybe we re-open to track?

@ryan-roemer
Copy link
Member

Opened #2321 to fix!

@becca-bailey
Copy link
Contributor Author

Thanks @ryan-roemer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Infrastructure 🏠 Requested changes that won't affect core functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants