-
Notifications
You must be signed in to change notification settings - Fork 295
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
feat: init nodejs-helpers package, add bypass-import-map, bypass-impo… #2053
Conversation
🦋 Changeset detectedLatest commit: 122a481 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
bd0bf36
to
0f3f1a5
Compare
9b29e2c
to
d1d028c
Compare
}; | ||
|
||
describe('bypassExportMap simple export map case', async () => { | ||
const packageDir = 'test-node/fixtures/simple-export-map'; |
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.
Did you consider using mock-fs (for readability and perf)?
Then we can do:
mockFs({
'/simple-export-map/components/icon/xyz.js': "export x = () => 'bla';"
// ... etc
})
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 did consider mocking filesystem, however it is another layer of abstraction on top of the reality, which will make it less readable imo, and more of a complex solution. Of course, as you suggested we could still consider it, say for performance, and that would certainly be the case, if we were mocking timers and so on, yet performance wise the whole tests that access the filesystem are taking ~0.247 seconds to be precise.
To get the ~0.247, we can
First run the tests without the tests which uses the filesystem
rm -rf packages-node/nodejs-helpers/test-node/tasks/ && time npm run test -w packages-node/nodejs-helpers/
real 0m1.323s
And then, run it with them:
g checkout packages-node/nodejs-helpers/test-node/tasks/ && time npm run test -w packages-node/nodejs-helpers/
real 0m1.570s
Then the difference is ~0.247,, which is insignificant.
* @returns {Promise<any>} | ||
*/ | ||
export const bypassImportMap = async (packageDir, options = {}) => { | ||
const ignoredDirs = options.ignoredDirs || ['node_modules']; |
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 a future improvement: maybe we can leverage globby's gitignore option?
const exportMapValuePath = path.join(packageDir, exportMapValue); | ||
const exportMapKeyPath = path.join(packageDir, exportMapKey); | ||
const searchPattern = isDirectorySync(exportMapValuePath) | ||
? path.join(exportMapValuePath, '**', '*.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.
At some point, we might also support css import assertions. At that point, we can probably extract the generic export map resolver from providence and use it here as well.
…rt-map, and run part of lion prepublish
d1d028c
to
122a481
Compare
…rt-map tasks, and run part of lion prepublish
What I did