[internal] Add more injection points #121
Conversation
yoannmoinet
force-pushed
the
yoann/add-more-injection-points
branch
from
a331ba9 to
bbe728b
Compare
yoannmoinet
force-pushed
the
yoann/add-more-injection-points
branch
from
84950be to
74d2575
Compare
| @@ -24,12 +24,14 @@ | |||
| }, | |||
| "dependencies": { | |||
| "async-retry": "1.3.3", | |||
| "chalk": "2.3.1" | |||
| "chalk": "2.3.1", | |||
| "glob": "11.0.0" | |||
There was a problem hiding this comment.
FYI node now supports OOTB glob (but it requires specific node version)
I'm not asking to change it, but just sharing knowledge hehehe
There was a problem hiding this comment.
Oh that's super nice, I didn't know.
I'm planning on bumping node version at some point this quarter.
There was a problem hiding this comment.
Edit: it's still experimental TIL – link
| if (!filepath.includes('*')) { | ||
| return [filepath]; |
There was a problem hiding this comment.
I don't think this is enough to know if this is a valid path / something that has to be globed. For instance hello.{js,ts} has to go through glob
There was a problem hiding this comment.
It's a subset of glob for eslint.
From the link in the comment above, it only support * and /**/.
| entryPaths.push( | ||
| ...Object.entries(entryPoints).map(([name, filepath]) => ({ name, path: filepath })), |
There was a problem hiding this comment.
Idk if this can happen, but if entryPoints is null, this will crash (typeof null is object)
There was a problem hiding this comment.
Good catch, it can happen (according to the types).
| // Resolve all the paths. | ||
| const proms = entryPaths | ||
| .flatMap((entry) => | ||
| getAllEntryFiles(entry.path).map<[{ name?: string; path: string }, string]>((p) => [ |
There was a problem hiding this comment.
Pro tip: you can label tuples (not a request for change, just knowledge sharing):
getAllEntryFiles(entry.path).map<[entry: { name?: string; path: string }, path: string]>((p) => [
| }, | ||
| ...rollupPlugin(), |
There was a problem hiding this comment.
You may want to put ...rollupPlugin() before config(config) { to avoid having a field name config in rollupPlugin() overriding your config
| onResolve: jest.fn(), | ||
| onLoad: jest.fn(), | ||
| onDispose: jest.fn(), | ||
| ...options, |
There was a problem hiding this comment.
You may want to move ...options to the 1st element to avoid being able to override the other fields. And if you want that to be its behavior, you should add a comment saying "All the methods before can be overridden in options" or something like that
There was a problem hiding this comment.
Changed options for overrides.
[tests] A better sandboxing of tests and builds
yoannmoinet
requested review from
jckr
and removed request for
a team and
jckr
What and why?
Here we are again, rewriting the injection plugin for the 18th time.
We'd need to inject code banner and footer, but also within the context of the bundle, being able to add new
importinstructions for instance.Especially for [rum] Inject browser sdk
How?
Some small update and fixes:
Big refactor of the injection plugin:
START,MIDDLEandEND.Note
Tests are refactored in #123