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: force the use of posix path separators for ts module graphs #8149

Merged
merged 8 commits into from Jun 13, 2022
Merged

Conversation

GeeWizWow
Copy link
Contributor

@GeeWizWow GeeWizWow commented May 26, 2022

↪️ Pull Request

When resolving the name of the main module for use in TS module graphs, ensure we force the usage of posix paths as this is what typescript uses and returns.

This fixes an issue when running on Windows, where an assert would be thrown as a result of two identical paths using different path separators being compared for equality.

For some reason I'm unaware of, this only occurs if a tsconfig.json file with a paths entry is present in the root folder.

💻 Examples

The main module name for ts module graphs is resolved here:

let mainModuleName = path
.relative(program.getCommonSourceDirectory(), asset.filePath)
.slice(0, -path.extname(asset.filePath).length);
let moduleGraph = new TSModuleGraph(mainModuleName);

On windows, this returns a path with dos path separators \.

Subsequently in collect.js when we use typescript to visit a node, it returns the node's name using posix separators /, when we add that node to the module graph:

moduleGraph.addModule(node.name.text, _currentModule);

It then attempts to compare it to the main module name resolved in TSTypesTransformer earlier:

addModule(name: string, module: TSModule) {
this.modules.set(name, module);
if (name === this.mainModuleName) {
this.mainModule = module;
}
}

Which fails due to the path separator mismatch.

Fast forward a bit, and getAllExports is called, and the assert is thrown due to nullthrows(this.mainModule), still being undefined/ null.

🚨 Test instructions

  • Ensure you are running on windows.
  • Follow the instructions for building a library (source, main and types entries in package.json)
  • Ensure your source file is a typescript file that exports from another module, for example:
export * from './library.ts';
  • Have tsconfig.json file in the same directory as your package.json. This file can contain anything, as long as it contains an arbitrary entry for compilerOptions.paths
  • Run parcel build

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

GeeWizWow and others added 2 commits May 26, 2022 23:00
When resolving the name of the main module for use in TS module graphs,
ensure we force the usage of posix paths as this is what `typescript`
uses and returns.
This fixes an issue when running on Windows, where an assert would be
thrown as a result of two identical paths using different path
seperators being compared for equality.
@mischnic
Copy link
Member

There are some existing utils we have for this operation:

import {normalizeSeparators} from '@parcel/utils';

Could you add a test case to this file? We do run a CI job on Windows so that would cover this bug

describe('typescript types', function () {

@GeeWizWow
Copy link
Contributor Author

GeeWizWow commented May 27, 2022

Thanks @mischnic I'll pick up those tasks tomorrow.

Can you shed any light into why this code path is only triggered in the presence of a tsconfig.json file containing a paths entry? Keen to understand the issue further...

@GeeWizWow
Copy link
Contributor Author

Updated with the above comments:

  • TSTypesTransformer now uses normalizeSeparators from @parcel/utils
  • Added an integration test in packages\core\integration-tests\test\ts-types.js

@GeeWizWow
Copy link
Contributor Author

GeeWizWow commented May 31, 2022

Test failure with:

 1) typescript types
       should handle a tsconfig file with paths on windows:
     Failed to resolve './lib/index' from './index.ts'

Will investigate.

Edit: ✅

@devongovett devongovett merged commit 58bd6d7 into parcel-bundler:v2 Jun 13, 2022
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.

None yet

3 participants