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

do not error when processing sources that include node hashbang #217

Merged
merged 5 commits into from
Jul 28, 2023

Conversation

iambumblehead
Copy link
Owner

cc @koshic @tommy-mitchell re #215

this PR removes the hashbang line from the sources before prepending import expressions, then re-adds any hashbang line to the final result returned by the loader

@iambumblehead
Copy link
Owner Author

@tommy-mitchell thank you for giving esmock your valuable attention. Feel free to leave any comments or advice.

This PR builds on the test from your earlier PR

@iambumblehead
Copy link
Owner Author

this test is failing in windows and maybe a different PR should resolve it. It relates to filesystem paths

"test-metaresolve": "rimraf dist && tsc && cross-env \"NODE_OPTIONS=--experimental-import-meta-resolve --loader=esmock\" NODE_NO_WARNINGS=1 ava",

@iambumblehead
Copy link
Owner Author

the win32 could probably be resolved soon, but slow internet speeds here make it difficult to setup a windows virtual machine to run tests with latest node 18

@tommy-mitchell
Copy link
Collaborator

I have a Windows x64 machine, would that work for reproducing? I could try resolving it

@iambumblehead
Copy link
Owner Author

@tommy-mitchell that would be great. Here is where I think the problem is. I think import.meta.resolve is returning the path n the wrong format

const resolve = isMetaResolve ?
  (import.meta.resolve.constructor.name === 'AsyncFunction'
    ? async (id, p) => import.meta.resolve(id, asFileURL(p))
      .catch(() => resolvewith(id, p))
    : (id, p) => {
      try { return import.meta.resolve(id, asFileURL(p)) }
      catch { return resolvewith(id, p) }
    })
  : resolvewith

cd into the tests-source-map folder and npm install then npm test to run the tests

@iambumblehead
Copy link
Owner Author

possibly related nodejs/node#35518

Screen Shot 2023-07-27 at 8 43 19 PM

@iambumblehead
Copy link
Owner Author

Pinning the node version number to 18.16 resolved the pipeline error. A ticket for that node 18.17 issue is created here nodejs/node#48948 and so this PR for hashbang support is complete and ready for review

@iambumblehead
Copy link
Owner Author

if anyone has retro-active review comments feel free to leave those

@iambumblehead iambumblehead merged commit a9ac615 into master Jul 28, 2023
@iambumblehead iambumblehead deleted the resolve-failing-hashbang-test branch July 28, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants