db-migrate / node-db-migrate

Database migration framework for node
Other
2.32k stars 360 forks source link

Replace module imports with dynamic names to static #670

Closed alexeyoganezov closed 4 years ago

alexeyoganezov commented 4 years ago

Why:

To allow db-migrate usage in projects that use zeit/ncc or zeit/pkg for bundling. These tools cannot correctly analyze and bundle modules that relies on imports like require(path.resolve(__dirname, module)); that are used in ./lib/commands/index.js.

Implementation details:

Notes

commitlint-wzrdtales[bot] commented 4 years ago

There were the following issues with this Pull Request

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

commitlint-wzrdtales[bot] commented 4 years ago

There were the following issues with this Pull Request

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 58f7736cd18b46b82253d589d704d8a397900a73 into 4caaf3989e6fddec9423e1e6ca3f6176ff28c6dd - view on LGTM.com

new alerts:

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

alexeyoganezov commented 4 years ago

Nope, not stale.

wzrdtales commented 4 years ago

I wouldn't support this PR since it effectively destroys certain modularity and introduces too many unnecessary repetitions. Wouldn't it be a better solution if db-migrate could evaluate and emit all resources that may be loaded for such bundlers to bypass their shortcomings?

wzrdtales commented 4 years ago

@alexeyoganezov I will release a version that features an optional static loader that can be activated with an option switch, or environment switch. That should resolve this problem and still not affect other components of db-migrate.

wzrdtales commented 4 years ago

however that might still result in problems with plugin loading and other stuff. plugins can be already plugged in from the outside though.

wzrdtales commented 4 years ago

I backported another change to 0.11.x decided to include this one https://github.com/db-migrate/node-db-migrate/commit/e18304686036b35da8b73b2b8db3100c21f2791d

so with the option switch staticLoader set to true a static loader can be used, was tested with zeit/ncc.

alexeyoganezov commented 4 years ago

Thanks a lot. Will try this feature on the next project.