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

moduleDetection: auto makes cjs/cts files parse as modules, and ... #49268

Merged
merged 1 commit into from
May 26, 2022

Conversation

weswigham
Copy link
Member

module: node sets moduleDetection: force, per our discussion at yesterday's design meeting.

Fixes #49207

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Nice, no API change

@weswigham weswigham merged commit 67673f3 into microsoft:main May 26, 2022
@weswigham
Copy link
Member Author

Does @DanielRosenwasser want @typescript-bot cherrypick this into release-4.7 ?

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 26, 2022

Heya @weswigham, I've started to run the task to cherry-pick this into release-4.7 on this PR at 5d13ab2. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @weswigham, I've opened #49276 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request May 26, 2022
Component commits:
5d13ab2 moduleDetection: auto makes cjs files parse as modules, module: node sets moduleDetection: force
@robpalme
Copy link

Today, if a user has sloppy-mode CJS files, they can use "alwaysStrict: false" to ensure the emit remains non-strict to preserve semantics.

Does that escape hatch still work after this change? Should we have a baseline test to check this case?

(I'm not sure how much to care about this scenario - mostly I'm just checking that any changes here are intentional).

// @allowJs: true
// @outDir: ./out
// @moduleDetection: auto
// @filename: foo.cjs
Copy link
Member

Choose a reason for hiding this comment

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

Does this also need change to isExternalOrCommonJsModule i think during incremental buildInfo doesnt mark bar.js as affecting global scope and that is a bug ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Afaik that just reads the two marker fields, which are all that get set to determine module-ness.

Copy link
Member

Choose a reason for hiding this comment

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

can you add test and ensure that bar.js will be marked as affects global scope and foo.cjs as module so no affectsGlobalScope in buildInfo. I am pretty certain that it is broken right now.

@weswigham
Copy link
Member Author

Yeah - if you set moduleDetection to legacy you get our old extension-ignorant behavior.

DanielRosenwasser pushed a commit that referenced this pull request Jun 1, 2022
Component commits:
5d13ab2 moduleDetection: auto makes cjs files parse as modules, module: node sets moduleDetection: force

Co-authored-by: Wesley Wigham <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.cts files are should not share the same top-level scope
5 participants