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

chore: vendor Mitt & update project structure #6209

Merged
merged 16 commits into from Jul 14, 2020
Merged

chore: vendor Mitt & update project structure #6209

merged 16 commits into from Jul 14, 2020

Conversation

jackfranklin
Copy link
Collaborator

As discussed in #6203 we need to vendor our common dependencies in so
that when we ship an ESM build all imports point to file paths and do
not rely on Node resolution (e.g. a browser does not understand import mitt from 'mitt').

@paullewis
Copy link
Contributor

The standard pattern is to put it in a root folder under third-party or lib, and I think we should do that here rather than embedding it in the file structure. That way it is very clear that it has been vendored in without digging.

@jackfranklin
Copy link
Collaborator Author

The standard pattern is to put it in a root folder under third-party or lib, and I think we should do that here rather than embedding it in the file structure. That way it is very clear that it has been vendored in without digging.

I normally would agree but I want to make it clear that we only vendor things in that are needed in all environments. E.g. we're not vendoring in any node-only dependencies.

WDYT?

@mathiasbynens
Copy link
Member

I normally would agree but I want to make it clear that we only vendor things in that are needed in all environments. E.g. we're not vendoring in any node-only dependencies.

Could we achieve that by adding vendor/README.md stating as much?

@jackfranklin
Copy link
Collaborator Author

I just realised it will complicate our setup much more if we have code outside of src - because we generate that directly into lib, but if code is outside of that structure the lib directory will change.

Therefore, how do people feel about having src/vendor, with a README as @mathiasbynens suggested explaining the setup?

We could have vendor at the top level, but it will mean a fair bit of tweaking of the build system that doesn't feel worth the effort, to be honest with you.

@mathiasbynens @paullewis LMK what you think

@mathiasbynens
Copy link
Member

I'd be alright with src/vendor or src/third_party, possibly with a symlink to it at the actual root for extra discoverability. Paul, would that address your concerns?

We could have vendor at the top level, but it will mean a fair bit of tweaking of the build system that doesn't feel worth the effort, to be honest with you.

With vendor/third_party at the top-level + a symlink perhaps the build system wouldn't need any tweaks?

Copy link
Contributor

@paullewis paullewis left a comment

Choose a reason for hiding this comment

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

LGTM

@jackfranklin
Copy link
Collaborator Author

@mathiasbynens we ended up chatting it through and going for the top level vendor directory, which I do think is probably cleaner in the long run. We've had to do some tweaking of the configuration but actually it's probably a bit cleaner now, and has some more clear divides between all the various ways we use TS (docs, tests, source code).

@mathiasbynens
Copy link
Member

Nice! I'm happy when Paul's happy. Thanks for resolving this 👍

@mathiasbynens
Copy link
Member

(note: please change the commit message accordingly while squash+merging)

As discussed in #6203 we need to vendor our common dependencies in so
that when we ship an ESM build all imports point to file paths and do
not rely on Node resolution (e.g. a browser does not understand `import
mitt from 'mitt'`).
@jackfranklin jackfranklin changed the title chore: vendor Mitt into src/common/third-party chore: vendor Mitt & update project structure Jul 14, 2020
@jackfranklin jackfranklin merged commit f1a6b8d into main Jul 14, 2020
@jackfranklin jackfranklin deleted the vendor-mitt branch July 14, 2020 15:57
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.

None yet

4 participants