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

Issue a warning if we detect a .js/.ts file collision #1046

Merged
merged 3 commits into from
Jan 21, 2020

Conversation

dfreeman
Copy link
Member

This PR will cause a warning to be emitted if we detect the situation described in #780. Because of exactly the situation we're detecting, we can't tell where the two conflicting files are coming from, but in most cases it will be apparent from context (e.g. the host app overriding a service from a particular addon).

Trying to describe the full situation and its possible ramifications would produce a giant wall of text in the terminal output, but the link in the warning points to #780, which has some good context and also links out to additional issues, like the upstream one in broccoli-persistent-filter.

image

@lifeart
Copy link

lifeart commented Jan 20, 2020

also issue happens if both files is .ts

way to escape issue:

  • both files is .js
  • one file with any extension

@dfreeman
Copy link
Member Author

also issue happens if both files is .ts

I don't think this is correct. Addons should never have .ts files in their app/ trees, but if they do and the host also has a .ts file, it will override the addon's file.

@lifeart
Copy link

lifeart commented Jan 20, 2020

#724

@dfreeman
Copy link
Member Author

Nothing in that thread indicates an issue when there were two conflicting .ts files when there wasn’t also a .js file of the same name floating around (e.g. because of differing addon versions that switched from including a .ts file to including a .js one)

If you can provide a reproduction where a nondeterministic build results from only .ts files I’m happy to take a look, but if you consider the underlying problem here, that can’t be caused by the same root issue as the one this PR is detecting, so it would need to be investigated and remedied separately.

Copy link
Member

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Looks good overall. Had one question just for my own understanding! :shipit:

ts/addon.ts Show resolved Hide resolved
ts/tests/helpers/skeleton-app.ts Show resolved Hide resolved
@lolmaus
Copy link

lolmaus commented Jan 21, 2020

Just tried it and it detected collisions:

WARNING: Detected collisions between .js and .ts files of the same name. This can result in nondeterministic build output; see https://git.io/JvIwo for more information.
  - uni-frontend/middleware/index.{js,ts}
  - uni-frontend/models/saved-configuration.{js,ts}
  - uni-frontend/reducers/index.{js,ts}

This is awesome, thank you so much!!

Copy link
Member

@jamescdavis jamescdavis left a comment

Choose a reason for hiding this comment

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

💯

@jamescdavis jamescdavis merged commit de45ac1 into master Jan 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the warn-on-js-ts-collision branch January 21, 2020 16:54
@jamescdavis
Copy link
Member

This has been released in v3.1.3.

@azhiv
Copy link

azhiv commented Aug 13, 2021

Can this warning be treated as an error during build? It would be nice to fail the build in case of a collision because otherwise proceeding with the collision (if missed) may result in significant harm.

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

6 participants