-
Notifications
You must be signed in to change notification settings - Fork 50
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
fix: Repair problem in package.json where require/import was swapped. Add in export of package.json #633
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.
LGTM
Switched this to draft, it turns out when you actually try to access INTERFACE_IDS node does more checking and fails with various errors during the typescript import. file:///private/var/folders/vp/5m13m6sx52g2y7zn2f006jdh0000gn/T/tmp.eHcBU7OO/dist/test.js:9
import { INTERFACE_IDS } from '@lukso/lsp-smart-contracts';
^^^^^^^^^^^^^
SyntaxError: Named export 'INTERFACE_IDS' not found. The requested module '@lukso/lsp-smart-contracts' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:
import pkg from '@lukso/lsp-smart-contracts';
const { INTERFACE_IDS } = pkg; |
…n't exactly do the right thing unless bundled for browser
It turns out the tsc solution was randomly working and not working. Neither node or ts-node properly support ESMs and when actually accessing the imported values random errors would occur. The best approach was to use esbuild with the --bundle flag to get a properly configured esm and run that. |
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.
Nice! This is great and very useful to have this import checks in the CI! 🚀
I have added some review comments with some questions.
"types": "./dist/constants.d.ts" | ||
}, | ||
"./userdocs/*": "./userdocs", | ||
"./devdocs/*": "./devdocs/*", | ||
"./artifacts/*": "./artifacts/*", | ||
"./dist/*": "./dist/*" | ||
"./dist/*": "./dist/*", | ||
"./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.
Why is the package.json exported 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.
Is it to export the version
field of the package.json?
If that's the case, maybe we can add a const VERSION = 0.10.2
in the constants.ts
file instead. As I am not sure about adding the package.json
in this list of files.
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 are currently using package.json and a lot of npms exports it. I think adding VERSION wouldn't be bad, but I don't think it's bad to export package.json either. It also makes this compatible with the current relayer code.
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.
Here is in article. They also suggest if downstream needs package.json to add it to export. https://formidable.com/blog/2021/node-esm-and-exports/
@@ -41,6 +41,9 @@ jobs: | |||
contracts.ts | |||
key: ${{ github.run_id }} | |||
|
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.
Ok very interesting. So you added a step to test that the constants can be imported successfully?
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.
Yes, this will do an import and require (i.e. esm and cjs) and validate that they both work.
cd $mktmp | ||
|
||
echo -e "${YELLOW}Creating package.json type=module${ENDCOLOR}" | ||
cat > package.json <<EOF |
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.
@richtera what does this line do exactly?
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 heredoc ending with a line containing just EOF being piped into package.json.
What does this PR introduce?
🐛 Bug: The package.json had the require and import sections
Add package.json into the export sections, which was missing.
PR Checklist
npm run lint
&&npm run lint:solidity
(solhint)npm run format
(prettier)npm run build
npm run test