-
Notifications
You must be signed in to change notification settings - Fork 3
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
Major version proposal #26
base: main
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.
left some general questions and comments
# Unreleased | ||
|
||
- BREAKING CHANGES | ||
- Update exports to support ES6 syntax |
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 is a lot for one PR. Ideally, I would prefer each of these entries to be a separate PR so that it's easier to understand what's changing and why in the commit.
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.
I do agree that it seems like a lot, but it's hard to break this up into smaller PRs because the changes all rely on each other. The description should give a clear overview of what's changing, but I still recommend cloning this branch and looking at the file structure locally. No functionality was changed to loadScript
and loadStylesheet
which is essentially the only 2 things this library is doing.
@@ -35,23 +42,14 @@ | |||
"eslint": "^8.47.0", | |||
"eslint-config-braintree": "^6.0.0-typescript-prep-rc.2", | |||
"eslint-plugin-prettier": "^5.0.0", | |||
"jest": "^29.6.3", | |||
"jest-environment-jsdom": "^29.6.3", | |||
"happy-dom": "^10.11.2", |
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.
what's happy-dom and why are we using it?
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.
It's a JS implementation of a web browser (mocking one essentially) that we use for running our tests. Can use this or jsdom.
tsconfig.json
Outdated
"strict": true, | ||
"target": "es5", | ||
"lib": ["es2015", "dom"] | ||
"noUncheckedIndexedAccess": 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.
can we sort these options alphabetically?
tsconfig.json
Outdated
"lib": ["es2015", "dom"] | ||
"noUncheckedIndexedAccess": true, | ||
"esModuleInterop": true, | ||
"skipLibCheck": 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.
Why are we setting this to true
? What are we skipping, 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.
Skipping any TS checks inside the node_modules
folder
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.
Can you merge in the main branch to include the changes that were made on Nov 29th? Also, resolve all conflicts?
JIRA
Summary
tsc
/src
directory.index.ts
file as the entry point to the library. This is a breaking change, and the import statements to use our modules will now change:import { loadScript, loadStylesheet } from @braintree/asset-loader
orvar loadScript = require("@braintree/asset-loader").loadScript
dist
directory directly from the source code.load-script.ts
andload-stylesheet.ts
tosrc/lib/
directoryWhy Vitest
Vitest is a "blazingly fast" unit test framework, and allows us to leave the test suite running in a watch-mode environment while we continue to make changes to our code. Only the necessary tests will re-run depending on what files were changed, making it much quicker to make changes without having to wait for the entire test suite to re-run.
Vitest has also been designed with a Jest compatible API, in order to make the migration from Jest as simple as possible. The API is essentially the same as Jest with some very minor differences.
Why Create
index.ts
as the entry pointdist
folder from the source codeWhy use Vite instead of
tsc
index.js
for ESM support andindex.umd.cjs
for CJS support.Checklist
Authors