Skip to content

Move Trix explicitly on to the window object#236

Closed
lemonmade wants to merge 1 commit intobasecamp:masterfrom
lemonmade:trix-to-window
Closed

Move Trix explicitly on to the window object#236
lemonmade wants to merge 1 commit intobasecamp:masterfrom
lemonmade:trix-to-window

Conversation

@lemonmade
Copy link

As mentioned in this issue, Trix currently does not work at all with node module loaders. The solution proposed in that PR specifically solved the issue for Webpack, but not for any bundler that wraps Trix in an IIFE (as most do). This is because the index file adds Trix to this, which is assumed to be window, and all other bundled files (themselves executed in IIFEs) assume the ability to access Trix somewhere in scope (which works when the original this was window, but not when it is anything else).

At Shopify, we're using a custom JavaScript bundle to be able to use npm packages. The only solution for us with Trix right now is to vendor it and make these changes manually.

Anyhow, this PR somewhat improves the situation by attaching Trix to window explicitly. This is far from ideal — it would be much better to use a UMD wrapper so that it can be brought into a project using AMD, CJS, or globals. However, this is a smaller change that isn't too much of a departure from what's currently there.

cc/ @javan

@mitar
Copy link
Contributor

mitar commented May 7, 2016

I am not sure if I like the idea of populating global window unnecessary. This should be optional.

@lemonmade
Copy link
Author

Like I said, this is not an ideal solution, the best solution is a UND wrapper but the issue proposing that was closed. This at least clarifies that Trix needs to be on window given the way it is currently written/ bundled, and allows it to work in cases where the context in which the script is executed is not the global context.

@mitar
Copy link
Contributor

mitar commented May 7, 2016

That is true. How the code references Trix internally is problematic.

@javan
Copy link
Contributor

javan commented May 10, 2016

I think this is a reasonable compromise so 👍.

Another minimal change would be to export Trix so it can be required as expected:

module?.exports = Trix

What do you think?

@mitar
Copy link
Contributor

mitar commented May 10, 2016

Why not just properly include an UMD wrapper?

@javan javan mentioned this pull request May 10, 2016
@javan
Copy link
Contributor

javan commented May 10, 2016

@mitar see your own comment and #119 (comment)

@lemonmade
Copy link
Author

module?.exports = Trix would work for my particular case as well, but it does feel like a bandaid rather than a real solution (and leaves AMD users out in the cold).

@javan
Copy link
Contributor

javan commented May 11, 2016

I'm open to supporting AMD too. If you're interested in adding "universal" support to this PR (or a new one), please do.

@lemonmade
Copy link
Author

No need for this anymore :) Thanks for the better fix!

@lemonmade lemonmade closed this May 18, 2016
@lemonmade lemonmade deleted the trix-to-window branch May 18, 2016 15:07
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