-
Notifications
You must be signed in to change notification settings - Fork 18
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
embed resolvewithplus #181
Conversation
@koshic @aladdin-add @tripodsan please review related discussions here #179 (comment) and here #176 (comment) |
It might be better if we do not enable |
the comment here indicates |
tests/package.json
Outdated
@@ -27,6 +27,9 @@ | |||
"mini:default": "cd .. && npx esbuild ./src/*js --minify --allow-overwrite --outdir=src", | |||
"mini:win32": "cd .. && cd src && forfiles /m \"*.js\" /c \"cmd /c npx esbuild @file --minify --allow-overwrite --outfile=@file\"", | |||
"mini": "run-script-os", | |||
"embed:cp-refd": "node --input-type=module -e \"import fs from 'fs';fs.copyFileSync('../node_modules/resolvewithplus/resolvewithplus.js', '../src/resolvewithplus.js')\"", | |||
"embed:update-refs": "node --input-type=module -e \"import fs from 'fs';let p='../src/esmockModule.js';fs.writeFileSync(p, fs.readFileSync(p, 'utf-8').replace(/'resolvewithplus'/, '\\'./resolvewithplus.js\\''))\"", | |||
"embed": "npm run embed:update-refs && npm run embed:cp-refd", |
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.
not sure I've gotten your point: npm run embed
should be run before publish, but I didn't see it was added.
I had a concern: the code in here is really fallible, and hard to debug. can we just copy the resolvewithplus
to a folder, and provide a script to update it to the newer version?
I made a similar thing in eslint-plugin-n
:
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.
npm run embed
is used by npm run test-ci
, also used by npm run prepublishOnly
,
combining parts of the main and test package.json scripts,
{
"embed:cp-refd": "node --input-type=module -e \"import fs from 'fs';fs.copyFileSync('../node_modules/resolvewithplus/resolvewithplus.js', '../src/resolvewithplus.js')\"",
"embed:update-refs": "node --input-type=module -e \"import fs from 'fs';let p='../src/esmockModule.js';fs.writeFileSync(p, fs.readFileSync(p, 'utf-8').replace(/'resolvewithplus'/, '\\'./resolvewithplus.js\\''))\"",
"embed": "npm run embed:update-refs && npm run embed:cp-refd",
"mini:pkg": "npm pkg delete scripts devDependencies",
"test:all-ci": "npm run mini && npm run embed && npm run test:all",
"test-ci": "npm run test:install && npm run test:all-ci",
"prepublishOnly": "npm run lint && npm run test-ci && npm run mini:pkg"
}
If storing resolvewithplus directly inside the src folder is preferred that is OK with me. The above are complicated (a little bit) and ugly, but they are verified at the unit-test pipeline.
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.
a little ugly but not too terrible (for me)
embed:cp-refd
import fs from 'fs';
fs.copyFileSync('../node_modules/resolvewithplus/resolvewithplus.js', '../src/resolvewithplus.js')
embed:update-refs
import fs from 'fs';
let p='../src/esmockModule.js';
fs.writeFileSync(p, fs.readFileSync(p, 'utf-8').replace(/'resolvewithplus'/, '\'./resolvewithplus.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.
I slightly prefer storing in the src, to avoid the magic like embed:cp-refd
/embed:update-refs
, but let's see what others think. :)
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 one trusts these lines, they allow one to develop esmock without needing to manually sync resolvewithplus and without needing to add extra files. esmock can be developed and used with regular npm, while unit-tests and prepublishOnly automaticaly get the embedded result.
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.
@koshic @aladdin-add unusual "ref" scripts are removed and esbuild "bundle" is used. What do you think?
npx esbuild esmockLoader.js \
--minify \
--bundle \
--allow-overwrite \
--platform=node \
--format=esm \
--outfile=esmockLoader.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.
thank you for sparing me your time so far
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'd prefer two different entry points in package.json - one for '--loader', one for 'import esmock' inside test code. Why?
To avoid mixing code executed in different sandboxes for different purposes, and possible side-effects.
So, ie may look like '--loader esmock/loader' and 'import {xxx} from "esmock"' from regular code:
exports: {
"types": "...",
"import": {
".": "./dist/main.js",
"./loader": "./dist/loader.js",
}
}
There is one thing - the more the build result differs from the source files, the less sense it makes to test this result. As an example, for C++ code you will never write tests to check .exe file, right?
Here is the same: bundle(s) is not the best source for unit tests (that's why I use 'dist' dir in my example). It can be tested in kind of e2e tests ('loaded without errors', 'has exports A & B', etc.), but regular unit tests is about raw source by many reasons: you can have multiple output formats, project can migrate to TS, you may want to change bundler (and output) without touching unit tests, coverage collecting much more easy without sourcemaps re-mapping, and so on.
So, I propose to have independent 'source with unit tests' & 'bundled code with e2e tests' layers. And it a good point for discussion - but in a different PR. This is PR is ready to merge.
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'd prefer two different entry points in package.json - one for '--loader', one for 'import esmock' inside test code. Why?
The current interface using --loader=esmock
and import esmock from 'esmock'
is easier for users and documentation.
To avoid mixing code executed in different sandboxes for different purposes, and possible side-effects.
The loader will run in a separate thread soon and all tests currently pass for supported versions of node.
There is one thing - the more the build result differs from the source files, the less sense it makes to test this result.
The un-processed sources are tested at the code coverage pipeline, but only on the node18 image. This PR (and other PRs) did and do encounter bugs related to bundling and minification and it is useful to test these files before publishing them.
As an example, for C++ code you will never write tests to check .exe file, right?
This is true generally, but there is a unique context for esmock. Functional tests give coverage that is nearly 100% and exporting and unit-testing each function is not needed imho. Input and output results are predictable.
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.
@aladdin-add this PR waits for your approval :)
Why don't use 'readfilewithplus'? Every Node function / API can be replaced, the question in only about reasons. Personally for me, they are:
Speaking of meta resolve, it's 5 until we have proves for 1 or do not have additional requirements like dynamically changing conditions to be 3. In other words, you can consider this as 'new functionality and readfilewithplus as polyfill', which is sooner or later will be enabled by default. If you don't have obligations like 'code must not require maintenance in 10 years', signed in blood ) |
It's a kind of not true. Meta resolve supports imports, while Jest (and a lot of custom resolvers) doesn't support it, and TS gets such support a few months ago. The main advantage of native function - the highest probability that result will be aligned with other native functionality, and any deviations will be detected by community and fixed (or be well-known). At the page you provided I can t find any issues with import.meta.resolve. May be it doesn't work with workers? May be there are very special setup? May be someone just haven't enough developer skill? I don't know, really ) |
…-processed sources instead
250193a
to
7776862
Compare
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!
This PR embeds resolvewithplus inside the esmock package. To test the embedded result in windows-latest ci eval'd node scripts were used rather than
sed
andcp
commands.