-
Notifications
You must be signed in to change notification settings - Fork 315
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
chore: add d.ts file compilation to testkit #2585
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good!
"start-server": "yarn lerna run storybook --scope=monday-ui-react-core" | ||
}, | ||
"bugs": { | ||
"url": "https://github.com/mondaycom/vibe/issues" | ||
}, | ||
"devDependencies": { | ||
"@playwright/test": "1.45.3" | ||
"@playwright/test": "1.45.3", | ||
"typescript": "^4.4.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason for not using a newer version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used the same version as in the root project. I don't see a reason why not to use a newer version. @talkor do you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have 4.7.3 in the root and in core, let's stick to it, we will upgrade one day all of them
btw, I think you should automatically get typescript when the root of the monorepo has typescript installed as a dev dep, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YossiSaadi yes it should work this way, but that means that when you'll upgrade the TS version you'll have to deal with all packages in one go. If you want, you CAN set each package using its own TS version. This will allow you a more flexible approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, exactly
we do want to have the repo root with packages that should have the same version across all the packages, but were not there yet
eslint, typescript, prettier, etc.
but I think for now, @uziab can use the one from root (or remove the declaration entirely, up to him to decide, not critical for me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a few comments
also, in general, you're not using eslint, prettier, etc. can you add it sometime to the library? it might have ts, lint, or format errors that we're not catching currently
"start-server": "yarn lerna run storybook --scope=monday-ui-react-core" | ||
}, | ||
"bugs": { | ||
"url": "https://github.com/mondaycom/vibe/issues" | ||
}, | ||
"devDependencies": { | ||
"@playwright/test": "1.45.3" | ||
"@playwright/test": "1.45.3", | ||
"typescript": "^4.4.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have 4.7.3 in the root and in core, let's stick to it, we will upgrade one day all of them
btw, I think you should automatically get typescript when the root of the monorepo has typescript installed as a dev dep, no?
@@ -17,13 +27,14 @@ | |||
}, | |||
"scripts": { | |||
"test:e2e": "npx playwright test", | |||
"build": "tsc", | |||
"build": "tsc && tsc --project tsconfig.esm.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this for a different esm and cjs? why we need cjs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question, why the need for cjs came up? is this really needed?
"import": "./dist/esm/index.js", | ||
"types": "./dist/index.d.ts" | ||
}, | ||
"./package.json": "./package.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would create a @vibe/testkit/package.json entry
is this really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you just want to export the package.json, it gets exported by default anyway, so no need to declare it in files
or in exports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"start-server": "yarn lerna run storybook --scope=monday-ui-react-core" | ||
}, | ||
"bugs": { | ||
"url": "https://github.com/mondaycom/vibe/issues" | ||
}, | ||
"devDependencies": { | ||
"@playwright/test": "1.45.3" | ||
"@playwright/test": "1.45.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is playwright
's version locked by accident or is that intended?
"@playwright/test": "1.45.3", | |
"@playwright/test": "^1.45.3", |
"include": [ "buttons", "inputs", "navigation", "pickers", "popover", "text", "utils", "BaseElement.ts", "index.ts"] | ||
} | ||
"compilerOptions": { | ||
"lib": ["ESNext"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need also "dom" in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for e2e in general I believe you do
I'm not sure for testkit but I think so as well
"moduleResolution": "node", | ||
"resolveJsonModule": true, | ||
"strict": true, | ||
"experimentalDecorators": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really needed? do you use it?
"target": "es6", | ||
"module": "commonjs", | ||
"moduleResolution": "node", | ||
"resolveJsonModule": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you import json files in your testkit? needed?
"resolveJsonModule": true, | ||
"strict": true, | ||
"experimentalDecorators": true, | ||
"emitDecoratorMetadata": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, really needed?
Resolves #