-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
wip: build: publish also for module ESM with new libESM #16470
base: develop
Are you sure you want to change the base?
Conversation
packages/crypto-utils/package.json
Outdated
@@ -22,7 +22,8 @@ | |||
"files": [ | |||
"lib/", | |||
"CHANGELOG.md", | |||
"!**/*.map" | |||
"!**/*.map", | |||
"src/" |
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.
"files" field doesn't always contain the same lines. Do we have the ambition to unify it now across all the published packages?
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.
That is a good question. Yes I think this is a good opportunity for doing that. Let's add "!**/*.map"
and "CHANGELOG.md"
?
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.
changelog sounds good (btw isn't it included implicitly regardless of this files field)?
about the .map file I don't have any strong opinion but it is probably better to unify 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.
You are right. It looks like 'yarn npm publish' includes CHANGELOG.md even if not included in "files":[..]
in package.json. It is the same for README.md
, but I realized in some packages we do not even have a README.md in the package. Like: https://www.npmjs.com/package/@trezor/blockchain-link-types
I think we should add at least a little README with the name and short description of the package, for the packages that we publish they probably deserve that minimum presentation.
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.
In order to be consistent we either can add CHANGELOG.md
and README.md
to the "files":[..]
section in package.json or remove it from all of them and the effect will be the same since yarn npm publish
will publish them anyway unless we include the files in .npmignore
.
@mroz22 Any opinions regarding this? Adding CHANGELOG.md
and README.md
to the "files":[..]
section in all the publish packages or remove 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.
I like the idea of being explicit more so I would add something like "*.md",
to the "files":[..]
section.
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.
In order to be consistent...
yes, I don't have any strong opinion here on whether to rely on the default or list everything explicitly
I like the idea of being explicit more so I would add something like "*.md",
I wouldn't use any wildcards for md files, this could include stuff we don't actually want there.
, but I realized in some packages we do not even have a README.md in the package
good catch, lets please add them 🙏
doesn't it need more changes in package json? some standardized instructions for bundlers where they should look? On the other hand, a src folder in the root feels quite standard already 🤔 just brainstorming edit: for example stuff like this https://stackoverflow.com/questions/42708484/what-is-the-module-package-json-field-for |
1387664
to
8c2e6aa
Compare
Yes, that is if we want to build packages for different modules, maybe for some packages where that makes sense we should do it. I mean for packages that are subject of being used in diverse environments. |
I still fail to see what benefit it brings to our users to only have src added to the releases. 🤔 going through the node_moduels in our repo I can't see many src files with .ts files. Lets make one step back - what problem are we actually trying to solve by this? |
8c2e6aa
to
94d7010
Compare
"build:lib": "yarn g:rimraf ./lib && yarn g:tsc --build tsconfig.lib.json && ../../scripts/replace-imports.sh ./lib" | ||
"build:lib": "yarn g:rimraf ./lib && yarn g:tsc --build tsconfig.lib.json && ../../scripts/replace-imports.sh ./lib", | ||
"build:lib:cjs": "yarn g:rimraf ./lib && yarn g:tsc --build tsconfig.lib.json && ../../scripts/replace-imports.sh ./lib", | ||
"build:lib:esm": "yarn g:rimraf ./libESM && yarn g:tsc --build tsconfig.libESM.json && ../../scripts/replace-imports.sh ./lib" |
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.
You will need to update replace-imports.sh
scripts to update imports also in new folder and imports will be in ESM format so you will have to change regexp in file too
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, what about this https://github.com/trezor/trezor-suite/pull/16470/files#diff-a103ae608a95489227c717243bcc41253b4a71f7b6a7e1b4fedaea4b7f7f96ad
?
I added new optional argument that will use libESM
instead of lib
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.
Then we need to have all the @trezor/...
packages that we consume to produce the libESM
.
scripts/updateProjectReferences.ts
Outdated
@@ -144,5 +144,37 @@ import { getPrettierConfig } from './utils/getPrettierConfig'; | |||
process.exit(1); | |||
} | |||
} | |||
|
|||
// Copy references also to tsconfig.libESM.json if exists |
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.
You shoudn't need this, just extend tsconfig.lib.json
in root tsconfig.libESM.json
instead of base
file
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, so it sounds like a good idea. Do you mean extending the tsconfig.lib.json
from the package and just changing the module
and outDir
like here https://github.com/trezor/trezor-suite/pull/16470/files#diff-97fc10c343bde083841616cc3853cf0d7e98003d6a1b11aa36a7665e69678b0b ?
@@ -18,13 +18,14 @@ | |||
"Ethereum" | |||
], | |||
"sideEffects": false, | |||
"publishConfig": { |
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 am not sure if we want to delete this, in case someones bundler doesn't support exports
field, it will pick main: "src/index.ts"
which doesn't exists in published package
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 am not sure either but it sounds like good idea to keep both approaches. Seams there should not be problem with that.
"main": "src/index.ts", | ||
"files": [ | ||
"lib/" | ||
], | ||
"exports": { | ||
"node": "./lib/index.js", |
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.
node sure about syntax but I think there is something like "commonjs": "./lib/index.js"
which may be better
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, I am still polishing it. Getting some inspirations from https://nodejs.org/api/packages.html#package-entry-points
@@ -49,6 +49,7 @@ playwright-report | |||
# build outputs | |||
lib | |||
libDev | |||
libESM |
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.
Please check also other "ignore" files in monorepo root, there are many of them
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.
Right, updated ee0fb8c
819288e
to
ee0fb8c
Compare
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is a deprecated package?The maintainer of the package marked it as deprecated. This could indicate that a single version should not be used, or that the package is no longer maintained and any new vulnerabilities will not be fixed. Research the state of the package and determine if there are non-deprecated versions that can be used, or if it should be replaced with a new, supported solution. What are wildcard dependencies?Package has a dependency with a floating version range. This can cause issues if the dependency publishes a new major version. Packages should specify properly semver ranges to avoid version conflicts. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
🚀 Expo preview is ready!
|
Description
Adding source to the release NPM packaged. This is common nowadays so users of the packages can transpile it as they need. It also allow them to do tree shaking.
Related Issue
Resolve #16245