Skip to content
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

fix(dynamicInstantiate): works on Windows #59

Merged
merged 2 commits into from
May 14, 2020
Merged

fix(dynamicInstantiate): works on Windows #59

merged 2 commits into from
May 14, 2020

Conversation

yoursunny
Copy link
Contributor

closes #39

@yoursunny
Copy link
Contributor Author

This code was copied from the patched loader from my other project.
https://github.com/yoursunny/NDNts/blob/91ebd330ac972a08c3678e6ccedc4c2c76110af2/mk/esm-loader.mjs

This has been tested with the code shown in #45 description, under the following combinations:

  • Windows 10, git bash, Node 14.2.0, NPM
  • Windows 10, git bash, Node 14.2.0, PNPM
  • Windows 10, PowerShell, Node 14.2.0, NPM
  • Windows 10, PowerShell, Node 14.2.0, PNPM
  • Ubuntu 18 in WSL, bash, Node 14.2.0, NPM
  • Ubuntu 18 in WSL, bash, Node 14.2.0, PNPM

I also fixed npm run prettier on Windows.

@EntraptaJ
Copy link
Member

Any chance you can get the GitHub actions tests to include Windows as the base OS as part of this PR?

@yoursunny
Copy link
Contributor Author

Windows builds have been added.
It’s my first time writing GitHub Actions, and I’m surprised that it worked the first time.

strategy:
matrix:
node: ['13.9', '13.10', '13.11']
os: ['ubuntu-latest', 'windows-latest']
node: ['13.9', '13.10', '13.11', '14.x']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch on that one.

strategy:
matrix:
node: ['13.9', '13.10', '13.11']
os: ['ubuntu-latest', 'windows-latest']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping this was possible, my other option was to just duplicate the entire tests section with Ubuntu and Windows.

Comment on lines +72 to +83
const urlParts = url.split('/node_modules/');

const pathParts = moduleUrl.pathname.split('node_modules/');
const specifier = pathParts.pop()!;
const nodeModulesBase = pathParts.join('node_modules/');
// Extract the module name after node_modules.
const moduleName = urlParts.pop()!;

const nodeModuleUrl = new URL('node_modules', pathToFileURL(nodeModulesBase));
// With NPM, this is just top-level node_modules.
// With PNPM, this is the innermost node_modules.
const nodeModulesPath = urlParts.join('/node_modules/');

// Create a Node.JS Require using the `node_modules` folder as the base URL.
const require = createRequire(nodeModuleUrl);
// Create a require function next to node_module, and import the CommonJS module.
const require = createRequire(`${nodeModulesPath}/noop.js`);
let dynModule = require(moduleName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be honest, having hard time figuring out the fuckery going on here, but it works, so I guess it's good.

@EntraptaJ EntraptaJ merged commit ce981c9 into K-FOSS:master May 14, 2020
@KJDev-Bot
Copy link
Contributor

🎉 This PR is included in version 1.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import of modules not working on windows: Path not formatted correctly
3 participants