Proper import chain for Meteor packages to app #6931
Conversation
#57) * move stuff from hot to modules-runtime-hot to run sooner * clean up duplicate methods between these two packages * don't try resolve moduleIds when adding to modulesRequiringMe rather do this lazily and try match with different patterns requires meteor/meteor#6931.
|
|
||
| let mainModule = _.find(info.files, file => file.mainModule); | ||
| mainModule = mainModule ? | ||
| `meteor/${name}/${mainModule.targetPath}` : false; |
There was a problem hiding this comment.
This can just be mainModule.installPath, I think, since that includes meteor/${name}/ prefix already.
There was a problem hiding this comment.
Thanks for the quick look! installPath isn't available yet, since currently the code runs just before the import-scanner. Would you prefer to move it to after? The import scanner takes the existing map as input, so not sure if any side effects here, or on mutating the generated scannerMap.
I did a quick test and started getting "Can't find npm module" errors for e.g. 'meteor/underscore', but I might have rushed too much. Also installPath is like node_modules/meteor/... so would need to either strip the node_modules/ or prefix with /.
|
Hope the tests are ok. Happy to rename the two new packages (in |
…g (#57) * move stuff from hot to modules-runtime-hot to run sooner * clean up duplicate methods between these two packages * don't try resolve moduleIds when adding to modulesRequiringMe rather do this lazily and try match with different patterns requires meteor/meteor#6931.
…g (#57) * move stuff from hot to modules-runtime-hot to run sooner * clean up duplicate methods between these two packages * don't try resolve moduleIds when adding to modulesRequiringMe rather do this lazily and try match with different patterns requires meteor/meteor#6931.
gadicc
changed the title
|
@benjamn, this would be appropriate for 1.3.3 I think, yeah? In light of my earlier comments, I think better to leave the code as is. Re failing tests, they're not mine: Unfortunately that last test breaks the CI process, but locally, my test passes. It's possible to squash directly via github merge now, but happy to rebase if you prefer (I guess that would trigger a new CI run, iirc you and others have made the CI testing more stable recently). |
|
@gadicc Please rebase. While 2-3 reruns of the old (flakier) tests would probably go through, a rebase will almost certainly work the first time and will trigger the build. |
Don't include the ", false" for install() on traditional packages add tests
gadicc
force-pushed
the
package-mainModule-import
branch
from
91045d7
to
dfee727
Compare
|
@abernix, done; tests passing :D definitely big improvements in testing... and this community issue management is a game changer! thanks! |
|
Yes that's amazing! Great work 👍 |
Addresses #6907.
@benjamn, this is what I had in mind and solves the issue for me. Pending any comments from you I'll play with it for a few more days then add some tests to the PR and rebase.